InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

ENH: Promote `MRFImageFilter::SetClassifier` to virtual and `m_ClassifierPtr` to protected

Open
#6,727 0 comments 0 reactions 0 assignees View on GitHub
area:Segmentation type:Design type:Enhancement
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

`itk::RGBGibbsPriorFilter` still carries a shadow declaration of `m_ClassifierPtr` because `itk::MRFImageFilter` keeps that member `private` and its `SetClassifier` is non-virtual — so the subclass method *hides* rather than overrides, and the two classes maintain independent classifier pointers with divergent `SetClassifier` semantics.

Follow-up to #6254, which removed the two `NumberOfClasses`/`MaximumNumberOfIterations` shadows in the same class but deliberately deferred this one because the fix requires a base-class API change.

Evidence (file:line, verified against main 2026-07-29)

`Modules/Segmentation/MarkovRandomFieldsClassifiers/include/itkMRFImageFilter.h:421` — in the `private:` section that begins at line 384:

```cpp
typename ClassifierType::Pointer m_ClassifierPtr{};
```

`Modules/Segmentation/MarkovRandomFieldsClassifiers/include/itkRGBGibbsPriorFilter.h:216` — the shadow, same type:

```cpp
typename ClassifierType::Pointer m_ClassifierPtr{};
```

Both `SetClassifier` declarations are non-virtual and neither carries `override`:

```cpp
// itkMRFImageFilter.h:248
void SetClassifier(typename ClassifierType::Pointer ptrToClassifier);

// itkRGBGibbsPriorFilter.h:131
void SetClassifier(typename ClassifierType::Pointer ptrToClassifier);
```

Because the methods are non-virtual, dispatch is chosen by the **static** pointer type:

| Static type of pointer | Method invoked | Which `m_ClassifierPtr` is mutated | Throws on `NumberOfClasses <= 0`? | Forwards `SetNumberOfClasses`? |
|---|---|---|---|---|
| `MRFImageFilter *` | base | base's | yes | no |
| `RGBGibbsPriorFilter *` | subclass | subclass's | no | yes |

The bodies are genuinely different, not redundant:

```cpp
// itkMRFImageFilter.hxx
if ((ptrToClassifier.IsNull()) || (m_NumberOfClasses <= 0))
{
throw ExceptionObject(__FILE__, __LINE__, "NumberOfClasses is <= 0", ITK_LOCATION);
}
m_ClassifierPtr = ptrToClassifier;

// itkRGBGibbsPriorFilter.hxx:167-168
m_ClassifierPtr = ptrToClassifier;
m_ClassifierPtr->SetNumberOfClasses(this->GetNumberOfClasses());
```

Subclass `.hxx` references to the shadowed member (all resolve to the subclass copy today):
`itkRGBGibbsPriorFilter.hxx:38` (ctor init), `167`, `168`, `411`, `415` (commented out), `418`, `420`, `542`.

Suggested approach

This is **not** a mechanical shadow removal like #6254; it changes `MRFImageFilter`'s public API and member protection. Order of operations:

1. Make `MRFImageFilter::SetClassifier` `virtual`; add `override` to `RGBGibbsPriorFilter::SetClassifier`.
2. Promote `MRFImageFilter::m_ClassifierPtr` from `private:` to `protected:`.
3. Delete the subclass's `m_ClassifierPtr` declaration. The eight `.hxx` references then resolve to the inherited member.
4. **Decide the semantics deliberately.** Once dispatch is virtual, a call through an `MRFImageFilter *` reaches the subclass override, which skips the base's `NumberOfClasses > 0` invariant and adds a `SetNumberOfClasses` forward. Either the base invariant should be enforced in the override, or the divergence must be documented as intentional. This is the review-blocking design question.
5. Audit for callers that rely on today's static-dispatch behaviour (likely none in-tree, but out-of-tree subclasses of `MRFImageFilter` exist).

Characterization GTests to land **first**, against unmodified code (the test-first protocol used for #6254/#6255):

- `MRFImageFilter.SetClassifierThrowsIfNumberOfClassesUnset` — pins the base invariant.
- `RGBGibbsPriorFilter.SetClassifierViaSubclassUpdatesClassifier`.
- `RGBGibbsPriorFilter.SetClassifierForwardsNumberOfClassesToClassifier` — pins the subclass's extra side effect.
- `RGBGibbsPriorFilter.SetClassifierViaBasePointerCurrentlyHidesSubclassMethod` — documents today's static-dispatch behaviour; update it to assert virtual dispatch after the change.

Related work

- #6254 (merged) — removed the `m_NumberOfClasses` / `m_MaximumNumberOfIterations` shadows in this same class; excluded `m_ClassifierPtr` as invasive.
- #6253 (merged) — `itkVirtual` / `itkFinal` / `itkNonVirtual` macro variants, useful for step 1.
- #6316 (merged) — `-Wshadow-field` enablement, which is what keeps surfacing this class of defect.
- Companion tracker for the rest of the sweep: see the shadow-member sweep issue (this repo).

Contributor guide

Open the contributing guide

Research direction

Start with Modules/Segmentation/MarkovRandomFieldsClassifiers/include/itkMRFImageFilter.h and itkRGBGibbsPriorFilter.h, then inspect the corresponding .hxx references. Add the four named characterization GTests first, including the current static-dispatch behavior. Done means the API change has deliberate invariant semantics, the shadow references are covered, and the relevant tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.