Inconsistency with Out of Range Behavior for Unsigned types

[awalter17]

Hello!

I noticed that certain Unsigned types in net.imglib2.type.numeric.integer clamp (i.e. if a number is negative set to zero, etc.) for values that are out of range for the particular type (i.e UnsignedIntType, UnsignedByteType, etc.), while others mask values that are out of range (i.e. Unsigned12BitType, Unsigned4BitType, etc.). Should all these types be consistent in their out of range behaviors, and if so which method should be used?

Thank you in advance for your responses and please let me know if any of that isn't clear!

[axtimwalde]

I think each type should do minimal work, i.e. on-set checking and clamping is generally not wanted because it can often be omitted. I understand that it hampers generality if every type does it's own crap and would instead suggest to introduce some contract interface? Then, typed generic code would potentially know.

[ctrueden]

@axtimwalde wrote:

I think each type should do minimal work, i.e. on-set checking and clamping is generally not wanted because it can often be omitted.

For performance, presumably?

Does that mean you would prefer that logic like this be removed, then?

I understand that it hampers generality if every type does it's own crap

FYI, @awalter17 encountered this issue when writing some unit tests for a branch of ImageJ OPS that she is currently working on, which adds ops that convert between types (e.g., convert.uint12(56)). So in this case, it creates more edge cases in the unit tests.

would instead suggest to introduce some contract interface?

Interesting, could you elaborate a little? Do you mean something like boolean isOutOfBounds() in Type or a subtype? RealType already has getMinValue() and getMaxValue() but they return double instead of T unfortunately, which as you know has limitations.

[axtimwalde]

Sorry for the incomplete comment yesterday. I would like to avoid to use setCodedSignedIntChecked instead of setCodedSignedInt per default because it performs extra work, similar in UnsignedByteType and UnsignedShortType. The more desirable masking in AbstractIntegerBitTypes is merely a side effect of how they are working. So we could simply say that there is no contract, users will have to make sure that clamping happens if they cannot make sure that input is within margins. RealType, for this purpose has the methods getRealMin() and getRealMax() which return doubles. I can see how this will not work for long or any integer longer than 52 bits. We could add methods realMin(T) and realMax(T) to RealType that would write their respective max and min values into a type that is inferred from the implementation? These types being comparable one could then do that clamping where necessary.

[awalter17]

I agree that it would probably be best to move away from clamping. Upon looking at the code for UnsignedIntType, UnsignedByteType, and UnsignedShortType the constructors each call getCodedSignedIntChecked( ) which performs clamping. However each of these classes also have a getCodedSignedInt( ) which performs masking. I would suggest that the constructors instead call getCodedSignedInt( ).

Additionally @ctrueden noted that the constructors each require a primitive value that is one level higher from the actual internal type (i.e. the constructor for UnsignedShortType requires an int value, etc.). It would be beneficial to have an additional constructor for each of these types that takes the internal value (so a constructor in UnsignedShortType that takes a short, etc.).

Does that sound like it could work? Please let me know your thoughts on this.