Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsupported datatype by Converter (ImagePlus) #60

Closed
StRigaud opened this issue Aug 8, 2024 · 8 comments · Fixed by #62
Closed

Unsupported datatype by Converter (ImagePlus) #60

StRigaud opened this issue Aug 8, 2024 · 8 comments · Fixed by #62

Comments

@StRigaud
Copy link
Member

StRigaud commented Aug 8, 2024

The array data type supported by ImagePlus is limited to uchar, ushort, and float. If, during some clesperanto pipeline, the return Array if of type int, the convertion back to ImagePlus will fail

Exception in thread "Run$_main" java.lang.IllegalArgumentException: Unsupported data type: uint
        at net.clesperanto.imagej.ImageJDataType.fromString(ImageJDataType.java:32)
        at net.clesperanto.imagej.ImageJConverters.copyArrayJToImgLib2(ImageJConverters.java:41)

We need to update the ImageJConverters.copyArrayJToImagePlus to cast the buffer result into a supported datatype.

@carlosuc3m This is important if we want to take advantage of the output management of clesperanto. I don't know if this can be an issue for the other converters

@carlosuc3m
Copy link
Member

What data type should we convert the int array into? Into float?

@StRigaud
Copy link
Member Author

StRigaud commented Aug 8, 2024

I think so, uint, int would go into float

@carlosuc3m
Copy link
Member

Great! I will implement it! What about the other data types?
Should byte go into ubyte? and short into ushort?

@StRigaud
Copy link
Member Author

StRigaud commented Aug 8, 2024

Yes, cast each unsuported toward the closest supported.

If that makes sense :)

@carlosuc3m
Copy link
Member

I am implementing this currently on a separate branch and i am wondering how to do it.
if we read an uint8 ArrayJ from the GPU, which values do we want that are represented in the image, the exact same values as in the arrayJ or the equivalent int8 values?

For example: if the ArrayJ contains the value 200, the pixel value in the image should be 200 (for which the ImagePlus data type used to represent it should be uint16), or it should be 200 - 256 = -56 (for which we would use the ImagePlus int8 image).

Also we have to note that java does not support unsigned datat types, so if the ArrayJ contained uint8 values, the buffer that Java will read will be directly converted to int8 doing the same as in the second proposal.

I would lean towards the second approach as it is what ImgLib2 does, but what do you think? @StRigaud

@carlosuc3m carlosuc3m linked a pull request Aug 13, 2024 that will close this issue
@tpietzsch
Copy link
Contributor

@StRigaud @carlosuc3m Could you point to a concrete example where that

Exception in thread "Run$_main" java.lang.IllegalArgumentException: Unsupported data type: uint
        at net.clesperanto.imagej.ImageJDataType.fromString(ImageJDataType.java:32)
        at net.clesperanto.imagej.ImageJConverters.copyArrayJToImgLib2(ImageJConverters.java:41)

exception happens? I need a bit more context.

I think, in general, it is best to let the user control what should happen:
promote to int16,
cast to int8 ignoring overflow (second proposal)
cast to int8 and clamp max to 127

I would lean towards the second approach as it is what ImgLib2 does, but what do you think? @StRigaud

I think it is a ok default approach.

Other options would be:
Just refuse to do that. Require the user to explicitly convert.

Yet another option:
If we don't want to burden users with these details and still avoid undesirable overflows etc, we could check the actual values and only fail if there are actually overflowing values. This would be a performance hit of course, but still something to consider.

@StRigaud
Copy link
Member Author

Hi @tpietzsch,

just to clarify the issue:
clesperanto buffer on GPU are type dependant (char, short, int, float, and unsigned equivalent). Hence, if we push a char type data, it should return a char type data. But if we do not specify any return information (e.g. provide null as output) the C++ code will detect this and allocate an adapted output memory space on the GPU for the processing. For memory logic and optimisation, we create uchar buffer and uint buffer for binary and label images, respectivaly. Otherwise, it return the same type as the input type.

On the CLIJ3 repo, while playing around, I tested a small pipeline where the output is not provided and connected_components_labeling return me an output buffer of type uint which cannot be converted back to readable fiji image.

To my knowledge, the issue relys on the datatype supported by the ImageJ converter:

public enum ImageJDataType {
FLOAT32(DataType.fromString("float"), ImagePlus.GRAY32, float[].class),
UINT16(DataType.fromString("ushort"), ImagePlus.GRAY16, short[].class),
UINT8(DataType.fromString("uchar"), ImagePlus.GRAY8, byte[].class);

And how the function to convert from clesperanto Array to ImagePlus is doing:

public static ImagePlus copyArrayJToImgLib2( ArrayJ arrayj )
{
long flatDims = arrayj.getHeight() * arrayj.getDepth() * arrayj.getWidth();
ImageJDataType dataType = ImageJDataType.fromString(arrayj.getDataType());
if (flatDims * dataType.getByteSize() > Integer.MAX_VALUE)
throw new IllegalArgumentException("The ArrayJ provided is too big to be converted into an ImgLib2 ArrayImg.");
ByteBuffer byteBuffer = ByteBuffer.allocateDirect((int) flatDims * dataType.getByteSize())
.order(ByteOrder.LITTLE_ENDIAN);
dataType.readToBuffer(arrayj, byteBuffer);
return fromBuffer(byteBuffer, dataType, arrayj.getDimensions());
}

From what I saw, @carlosuc3m already tried to work on this:

But I haven't tested yet if this works or not.

Other options would be:
Just refuse to do that. Require the user to explicitly convert.

Yet another option:
If we don't want to burden users with these details and still avoid undesirable overflows etc, we could check the actual values and only fail if there are actually overflowing values. This would be a performance hit of course, but still something to consider.

I am usually in favor of forbidding, especially in the context of a developper library. But because this concern a piece of code specific to ImageJ, and this piece of code is in charge of making the communication between ImageJ and clesperanto, It should ensure that what ever returns clesperanto, it can convert it to into an ImagePlus. if that make sense?

In general, clesperanto return the same type as you provide, with the exception of uchar and uint which are the only non supported type by ImageJ that clesperanto can return. My logic would be to cast to the closes type ImageJ accept, e.g. uchar to char, uint to float,

PS: my knowledge of Java and ImagePlus datastructure is quite limited, I don't know if what I am saying make sense 🙃

@carlosuc3m
Copy link
Member

@tpietzsch @StRigaud
To summarize a bit what is done for ImagePlus and see if you like it:

ImagePlus can only display images as float32, uint16 or uint8. Clesperanto supports int8, uint8, int16, uint16, int32, uint32 and float32.

This mismatch between data types is only problematic when pulling images from the GPU. In order to workarounf this problem, what is done in the ImageJDataType class is to pull:

  • uint8 as uint8
  • int8 as uint8 (converting negative values into uint8 by doing UINT8.max + pixel_value_int8)
  • uint16 as uint16
  • int16 as uint16 (converting negative values into uint16 by doing UINT16.max + pixel_value_int16)
  • int32 as float32
  • uint32 as float32
  • float32 as float32

The logic can be seen more clearly in the test class that checks if pulls are done correctly for ImagePlus:
https://github.com/clEsperanto/clesperantoj_prototype/blob/add-missing-types-imagej/src/test/java/TestPullImagePlus.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants