-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
[V3] Allow for incomplete codec metadata using numcodecs.get_codec #1447
[V3] Allow for incomplete codec metadata using numcodecs.get_codec #1447
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1447 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 37 37
Lines 15019 15026 +7
=========================================
+ Hits 15019 15026 +7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of making the handling of codec configuration the same in V2 vs V3.
I do think this needs a test to verify that the situation described in cgohlke/tifffile#211 is handled correctly after this change.
Test failures are unrelated and are fixed here #1450 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks for adding the test.
Once you note this change in docs/release.rst
and marge main to get the final test passing, I think we are good to go.
Any comments on this change from other devs? I'd be curious to hear @grlee77's take on relaxing the strict codec metadata expectations, as he is the one who wrote the original code here.
@zarr-developers/python-core-devs - looking for some final feedback here. If I don't hear any, I'll plan to merge ~7/12. |
LGTM. I suppose if the conf has invalid values, you get an exception when the codec is instantiated - do we have any expectation/test of how that should go? For the related issue in TIFFfile, I will comment that many codec options, specifically for compressors, are only actually important at compress time, not when reading. It would be useful to know, though, if the defaults in python implementations are compatible with whatever TIFF assumes - I don't know how to do that rigorously. |
The only compression method specified in the TIFF standard that has a codec in the numcodecs package is "deflate". The spec says that "each image segment consists of a single complete zlib data stream.". This is compatible what numcodecs.Zlib handles. All the other dozens of compression methods found in TIFF are either nowhere specified specifically for TIFF (e.g. LZMA, Zstd) , not supported by numcodecs (PackBits, LZW, CCITT, JPEG), or both (e.g., JPEG2000, WebP, PNG, JetRaw). The imagecodecs package provides many of those codecs made for TIFF. Few of them might require setting extra arguments, such as JPEG ( |
@jhamman - we just need release notes for this. That's important, since this change could hypothetically impact existing code. |
@cgohlke , thank you for the thorough reply. It seems we have nothing to worry about here. |
…x/v3-compressor-config
…x/v3-compressor-config
…thon into fix/v3-compressor-config
This PR refactors how codecs are instantiated in the current V3 implementation. This allows for default parameters to be omitted.
TODO:
closes #1443