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

Flex: ignore files where compression cannot be identified #4050

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

melissalinkert
Copy link
Member

Companion to #4040. This should fix test failures in the flex directory without requiring configuration changes.

Files with a compression type not in the TiffCompression enum (e.g. Lurawave) will now be skipped instead of throwing an exception.

I had initially added a continue between lines 1244 and 1245, but that causes more test failures as it changes the file grouping (since the else block starting in line 1283 is not executed).

@melissalinkert melissalinkert added this to the 7.0.0 milestone Jul 14, 2023
Copy link
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.

My understanding is that the current logic aims at separating flex files based on a series of criteria including compression. In particular, uncompressed as well as lwf-compressed flex files should never be grouped together under the same filesets. The current change semms to keep this logic working but as suggested in my inline comment, I wonder if we want to construct additional synthetic datasets with mixed compressions to cover all scenarios.

Together with #4040, this raises the question of expectations for the detection of Flex filesets. Reading of uncompressed flex filesets should obviously be unchanged. Should the lwf-compressed flex files still be initialized and fail openBytes or should they fully fail initialization?

firstIFD.getCompression() != TiffCompression.UNCOMPRESSED;
}
catch (EnumException e) {
LOGGER.trace("Could not get compression for " + file, e);
Copy link
Member

Choose a reason for hiding this comment

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

In practice the only two types of compressions we might have ever encountered with the Opera Flex format are UNCOMPRESSED and LURAWAVE, right?

Should compressed be set to True in that case to capture the fact the data is compressed and ensure minimal changes with regard to the compression comparison below?

Copy link
Member Author

Choose a reason for hiding this comment

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

curated/flex/karsten/Columbus_ExtendedData/Images/AFo_U2OS_20xW_070209/Meas_01(2007-02-09_15-36-40) (which we do not test) has undefined compression type 34692, but as far as I can tell the rest of our sample data is either uncompressed (type 0 or 1) or Lurawave (type 65535).

1f27a1a sets compressed to true here, and also removes a continue later on so that attempting to initialize a Lurawave-compressed file will fail with a more informative message. Lurawave-compressed files will throw an exception during setId now; with 6.14.0 and no Lurawave jar, the exception was thrown in openBytes. Since we do not support Lurawave compression at all anymore, I think throwing an exception during initialization is better, particularly as it will prevent importing unsupported data into OMERO.

The exception thrown when an unsupported compression type is encountered
should now be more informative.
@sbesson sbesson self-requested a review July 22, 2023 07:48
@sbesson sbesson merged commit 65db5eb into ome:develop Jul 25, 2023
17 checks passed
@melissalinkert melissalinkert deleted the flex-compression branch September 6, 2024 19:01
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.

2 participants