Skip to content

Don't alter OME-XML BigEndian values for int8/uint8 data#151

Open
melissalinkert wants to merge 1 commit intoglencoesoftware:masterfrom
melissalinkert:mixed-pixel-types
Open

Don't alter OME-XML BigEndian values for int8/uint8 data#151
melissalinkert wants to merge 1 commit intoglencoesoftware:masterfrom
melissalinkert:mixed-pixel-types

Conversation

@melissalinkert
Copy link
Copy Markdown
Member

The endianness coming from the Zarr library may be different in this case, since it doesn't technically matter what the endianness is for int8/uint8 data. However, this prevents the endianness from mismatching if a uint8 Image is followed by a uint16 (or greater) Image.

Without this change, relevant test data would have thrown a FormatException with the (incorrectly formatted) Endian mismatch... message. With this change, conversion should succeed. This may need a little reworking when we switch to zarr-java for v2 reading.

I wanted to add a test for this case, but the fake file structure doesn't currently allow creating multiple series with different pixel types, so will need to give that some more thought.

The endianness coming from the Zarr library may be different in this case,
since it doesn't technically matter what the endianness is for int8/uint8 data.
However, this prevents the endianness from mismatching if a uint8 Image is followed
by a uint16 (or greater) Image.
Copy link
Copy Markdown
Member

@Yajing826 Yajing826 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to successfully run the conversion and load the data into OMERO Plus. Thanks!

Copy link
Copy Markdown
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with a Zarr v2 dataset containing a mixture of arrays with "|u1" and ">u2 dtypes.

Without this PR, the conversion fails with the Endian mismatch error (including unreplace {} values) and it succeeds with this PR included.

A few questions:

  • should we wait for #152 to be included to retest this PR? should we also test it against v3 data?
  • has this always been an issue or is this a recent regression?
  • even though endianness is not relevant for single byte data types, should it also be the responsibility of bioformats2raw to try and maintain consistency between the underlying Zarr array metadata and Pixels.BigEndian?

In terms of testing, I assume it should be possible to create a sample OME-XML with a mixture of data types?

@melissalinkert
Copy link
Copy Markdown
Member Author

If an RC with this change is needed very urgently (defer to @Yajing826), then I would say test this first, and then double-check this case with v2 and v3 as part of the review of #152. If an RC with this change is not needed urgently, then it might be better to get #152 in first and then we can re-evaluate this.

  • has this always been an issue or is this a recent regression?

I believe this has always been an issue. It will only appear in the case where there is a int8/uint8 series before a higher byte depth image, which isn't super common with the data that we typically convert.

  • even though endianness is not relevant for single byte data types, should it also be the responsibility of bioformats2raw to try and maintain consistency between the underlying Zarr array metadata and Pixels.BigEndian?

It really depends on what happens in the underlying library. For jzarr:

https://github.com/zarr-developers/jzarr/blob/main/src/main/java/com/bc/zarr/ZarrHeader.java#L66-L72
https://github.com/zarr-developers/jzarr/blob/main/src/main/java/com/bc/zarr/ZarrHeader.java#L115-L117

So that forces writing int8/uint8 with undefined endianness in all cases, but will return the system-native endianness when reading the same data back. I don't see how we can work around that, but definitely worth re-evaluating for zarr-java.

@melissalinkert
Copy link
Copy Markdown
Member Author

And for zarr-java, note that the only int8/uint8 data types allowed for v2 are unspecified endianness:

https://github.com/zarr-developers/zarr-java/blob/main/src/main/java/dev/zarr/zarrjava/v2/DataType.java

@Yajing826
Copy link
Copy Markdown
Member

If an RC with this change is needed very urgently (defer to @Yajing826), then I would say test this first, and then double-check this case with v2 and v3 as part of the review of #152. If an RC with this change is not needed urgently, then it might be better to get #152 in first and then we can re-evaluate this.

It's not urgent, I only needed it for internal testing, and so far no customer has had this issue directly.

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 this pull request may close these issues.

3 participants