feat: Remove special cases from SegmenationMethod.from_dict

[peterhollender]

Removes the special-case handling for UniformWater and UniformTissue in SegmentationMethod.from_dict. Each subclass now overrides to_dict to exclude the ref_material attribute, which is implicit in the class and not a constructor argument.

SegmentationMethod.from_dict now checks for unexpected keywords and handles them based on the on_keyword_mismatch parameter. This allows for graceful handling of old JSON files that may contain the ref_material keyword.

Closes #362

[arhowe00]

The test cases look good. All I did was rename commit titles. I'll approve this for merging, feel free to comment on below if anything raises a concern:

1. There are many lines of code designed specifically to handle how ref_material being set, when what this field expresses can be made simpler. What the inheritance hierarchy means is clashing with what ref_material means (and it is settable in certain cases). We could remove the subclassing and ABC entirely, with everything being specified in a SegmentationMethod, or we could make this feature non-settable (even in constructors), make it class level for all specializations of SegmentationMethod.

2. I'm not sure whether the

   graceful handling of old JSON files that may contain the ref_material keyword.

   is an approach we should take for all things going forward, as it adds a lot of debt if we add handling to old versions of data. I think the project is small enough right now that we can keep the debt low and remove the field from the existing (small number) of JSON files. I'll approve it for merging anyway as this is cautionary but not a problem.

[ebrahimebrahim]

This is a really specific warning to be ignoring at the pyproject.toml level. If it's needed to get test_from_dict_on_keyword_mismatch to work properly, then I would say something is likely wrong with the test. Unless I misunderstood the purpose of it. It is a bit of a red flag

[peterhollender]

Hmm... pytest runs fine without it. A few of these PRs have been trial runs of Jules, so good to know that it takes some ... unconventional approaches. Looks like I'd already achieved the desired behavior, but this snuck past me

[peterhollender]

1. There are many lines of code designed specifically to handle how ref_material being set, when what this field expresses can be made simpler. What the inheritance hierarchy means is clashing with what ref_material means (and it is settable in certain cases). We could remove the subclassing and ABC entirely, with everything being specified in a SegmentationMethod, or we could make this feature non-settable (even in constructors), make it class level for all specializations of SegmentationMethod.

I suppose we could remove UniformWater and UniformTissue altogether, and make the user simply do UniformSegmentation(ref_material='water') or UniformSegmentation(ref_material='tissue'). I'll switch the definitions in my example data over, but hang on to these convenience classes for now

[peterhollender]

2. I'm not sure whether the

   graceful handling of old JSON files that may contain the ref_material keyword.

   is an approach we should take for all things going forward, as it adds a lot of debt if we add handling to old versions of data. I think the project is small enough right now that we can keep the debt low and remove the field from the existing (small number) of JSON files. I'll approve it for merging anyway as this is cautionary but not a problem.

I see the point here. I'm going to need to do a dvc update once the current set of changes come in, so maybe that's the point where I pull out this backwards-compatibility stuff

[ebrahimebrahim]

Sounds good! You can re-request review from me when done with additional changes

(we won't do any force-pushing till then -- the branch is in your hands, Peter. @arhowe00)

[ebrahimebrahim]

Heads up that I'll force-push to fix merge conflicts and commit messages