Closed CPBridge closed 2 years ago
Tagging @fedorov who first suggested this as an issue
Thanks @CPBridge. Let me think about the subclassing issue. It's conceptually nice for instances of all three classes to be an instance of MeasurementsAndQualitativeEvaluations
. The templates have some differences, but they are all included in the same Content Sequence.
@CPBridge I would prefer keeping the old class hierarchy. Couldn't the new parameters for get_image_measurement_groups()
be supported in a more dynamic way without having to change class inheritance?
Couldn't the new parameters for get_image_measurement_groups() be supported in a more dynamic way without having to change class inheritance?
The parameters for get_image_measurement_groups()
are fine, there's no need to change the class hierarchy for these.
The issue is these two things:
.source_images
property on MeasurementsAndQualitativeEvaluations
, which probably should not be inherited by its subclasses. We could always override it to just return an empty list or something, but that's a bit ugly.source_images
parameter of the constructor of MeasurementsAndQualitativeEvaluations
, which is not present in the subclass constructors, and thus breaks Liskhov. How much we care about this is an open question, I think we've broken it elsewhere, but I try to follow it unless there's a good reason not to.I'm not especially keen on one solution over the other, they are both a little ugly in my opinion. On balance I thought I'd go with changing the hierarchy, but we can go with the above instead if you prefer.
I see. Thanks for the clarification. I agree with your notion of following it unless we have a very strong reason not to.
Maybe we should move away from inheritance and start using Protocol. The class that defines the protocol (interface) can be decorated with @runtime_checkable and can be used as a common type.
What do you think about calling the base class or protocol class ImagingMeasurements
(that's the name of the parent content item)?
Maybe we should move away from inheritance and start using Protocol. The class that defines the protocol (interface) can be decorated with @runtime_checkable and can be used as a common type.
Are you talking about for future parts of the library, or for this PR? I'm a little reluctant to completely restructure this part (and only this part) of the templates code. The main reason for inheritance here is reducing code duplication, and Protocol
doesn't help there, right?
What do you think about calling the base class or protocol class ImagingMeasurements (that's the name of the parent content item)?
Sure, but we don't want it to be a public class, do we? So _ImagingMeasurements
?
Are you talking about for future parts of the library, or for this PR? I'm a little reluctant to completely restructure this part (and only this part) of the templates code. The main reason for inheritance here is reducing code duplication, and Protocol doesn't help there, right?
It doesn't help with that. However, it would give us a type so that we woulnd't have to write Union[MeasurementsAndQualitativeEvaluations, PlanarROIMeasurementsAndQualitativeEvaluations, VolumetricMeasurementsAndQualitativeEvaluations]
to refer to them collectively.
Sure, but we don't want it to be a public class, do we? So _ImagingMeasurements?
I was thinking to make it public to be able to refer to these "groups" collectively. They are very similar (at least conceptually) and (should) share a common interface (finding type, finding site, etc.).
@CPBridge as discussed, let's keep the base class private for the time being.
@hackermd Shall we merge this so that @fedorov can use it for his purposes on #164 ?
Address #164
By adding:
SourceImage
class inhighdicom.sr.content
MeasurementsAndQualitativeEvaluations
calledsource_images
that allows the user to pass a list ofSourceImage
objects.source_images
property on theMeasurementsAndQualitativeEvaluations
object, that allows the user to find the source image content items.MeasurementReport.get_image_measurement_groups
forreferenced_sop_instance_uid
andreferenced_sop_class_uid
to allow users to filter measurement groups by the referenced imagesNote that in order to do this I had to add things to the
MeasurementsAndQualitativeEvaluations
class that should not be present on thePlanarROIMeasurementsAndQualitativeEvaluations
andVolumetricROIMeasurementsAndQualitativeEvaluations
classes, and we find ourselves up against our old friend the Liskhov Substitution Principle. Therefore I changed the class hierarchy from this:to this:
Note that this is in a narrow way a break to the public API in that two public classes (Planar, Volumetric...) are no longer subclasses of another public class: MeasurementsAndQualitativeEvaluations.