Allow passing of additional type 3 attributes of Equipment module

[hackermd]

The attributes Series Number and Instance Number have type 2 (for most IODs I checked). Therefore, it should be allowed to pass None as arguments to the constructor of highdicom.SOPClass.

[hackermd]

I also added parameters for additional type 3 attributes of the Equipment module to the constructor of highdicom.SOPClass. These attributes are required for some IODs (e.g., Segmentation) and optional of others. Since they should be allowed in all SOP instances, it's nice if they can just be passed to the constructor.

[CPBridge]

Your are strictly correct of course. However I still favour making this a requirement, in an opinionated way. Instance number and series number are so ubiquitous that most people assume they are required and as a result I have seen many tools fail for no reason other than one instance somewhere in the study had no instance number/series number. This includes an unnamed "commercial grade" de-indentification tool....

Is there a good reason to omit these attributes?

I am on board with adding the other attributes to the base class constructor. However, if we do this this, we should remove code that sets them from the subclass constructors and always defer to the base constructor to handle them

[hackermd]

Is there a good reason to omit these attributes?

Yes! It is generally very difficult to select a good value for Series Number if multiple series are created for a given study in parallel. The same applies to Instance Number.

[CPBridge]

Note that instance number and series number are type 1 for Segmentation and Parametric Map. To me this both a practical reason to require them to avoid dealing with different requirements per IOD, and also an indication that, for newer IODs, the standards organisation want to push in the direction of them being required

[hackermd]

I am on board with adding the other attributes to the base class constructor. However, if we do this this, we should remove code that sets them from the subclass constructors and always defer to the base constructor to handle them

I wouldn't do, because they are optional in the context of the Equipment module, but required in the context of the Enhanced Equipment module. Therefore, I would keep the parameters for constructors of derived classes.

[CPBridge]

I wouldn't do, because they are optional in the context of the Equipment module, but required in the context of the Enhanced Equipment module. Therefore, I would keep the parameters for constructors of derived classes.

By all means keep the parameters, I'm just saying pass them through to the base class constructor for handling rather than setting the attributes on the subclass

[CPBridge]

Yes! It is generally very difficult to select a good value for Series Number if multiple series are created for a given study in parallel. The same applies to Instance Number.

It seems to me that there will always be some way around this (getting a thread ID somehow and making it a function of that) though it might be a little fiddly, and in my personal opinion this is not worth the confusion of omitting them. But we can do it if you disagree and handle the different requirements per IOD

[hackermd]

It seems to me that there will always be some way around this (getting a thread ID somehow and making it a function of that) though it might be a little fiddly, and in my personal opinion this is not worth the confusion of omitting them. But we can do it if you disagree and handle the different requirements per IOD.

I am not just talking about different threads. There may be a viewer running in a web browser that generates annotations and several models running on different machines that generate annotations in parallel.

I have started to assign random to Series Number and Instance Number, but that is does neither guarantee any uniqueness or particular meaningful order within the scope of the parent study or series. Therefore, I would just like to omit the values altogether.

Since the attributes are type 2 this is fully compliant with the standard. It's unfortunate if some libraries or applications struggle with reading the data, but this should not be our guiding design principle.

[CPBridge]

You realise that this PR only makes it optional on the base class and not on any of the implemented subclasses, is that your intention? I.e. it would only make a difference for those using their own subclasses of the base class (I assume this is what you are doing)?

We would also need to add runtime checks on the type of instance_number and series_number parameters for Segmentation and Parametric Map (and any other IODs where they are type 1, I only checked those two), otherwise we would allow the construction of non-compliant objects.

(You also always have the option of overwriting the attributes with None after construction).

[CPBridge]

We would also need to add runtime checks on the type of instance_number and series_number parameters for Segmentation and Parametric Map (and any other IODs where they are type 1, I only checked those two), otherwise we would allow the construction of non-compliant objects.

A more future-proof way of doing this may be to use the tools in _module_utils to determine on the fly whether the attributes are type 1 or 2 within the base class constructor based on the sop_class_uid. Just a thought

[hackermd]

We would also need to add runtime checks on the type of instance_number and series_number parameters for Segmentation and Parametric Map (and any other IODs where they are type 1, I only checked those two), otherwise we would allow the construction of non-compliant objects.

Oh boy. I didn't realize that there are IODs where these attributes are type 1. This is madness.

In that case, I may have to roll back what I said. If we have to come up with a number for some IODs then I agree with you that we can come with it for the other IODs, too.

[CPBridge]

Sorry @hackermd , I just noticed that on the current master branch, all the subclasses' docstrings suggest that series_number (but not instance_number) is of type Union[int, None] in contradiction to both the type hints in the function signature and the behaviour of the code.

I think we should probably fix this here...