-
Notifications
You must be signed in to change notification settings - Fork 54
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
Validate axes for writer #123
Conversation
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
==========================================
+ Coverage 70.10% 71.04% +0.93%
==========================================
Files 11 11
Lines 1104 1126 +22
==========================================
+ Hits 774 800 +26
+ Misses 330 326 -4
Continue to review full report at Codecov.
|
Hello @will-moore, that was quick, thank you for having a look! from ome_zarr.writer import write_image
import numpy
import zarr
store = zarr.DirectoryStore("/tmp/blah.zarr")
img_group = zarr.group(store=store)
img = numpy.zeros((123, 456, 1), dtype="uint8")
# now knowing that this axisorder is not supported
# I'd still expect a meaningful Exception here...
# not LinAlgError: SVD did not converge
write_image(img, img_group, axes="yxc") while naive, I think this looks like a totally innocent usage of the library. Adding the note in the documentation is of course an improvement that could potentially save some users of the library of running into the same problem. |
@k-dominik Thanks for testing. |
@k-dominik I pushed a fix where I test for size_x or size_y being 1 before scaling.
|
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.
A few minor suggestions mostly revolving around providing user feedback otherwise I think this is a useful feature as it isolate the axes
metadata validation and adds some coverage of the various scenarios.
@k-dominik do you also want to have a look and confirm this addresses #122 ?
In terms of releases, this effectively adds a new public API so that would be 0.3.0
or should we make the validation API internal for now with a leading underscore and call this 0.2.1
?
@sbesson fixes pushed, except for |
e16de4d
to
b33e9f0
Compare
Hello @will-moore and @sbesson, thank you guys so much for looking into it. I checked out the latest changes and now if I come with my little badly ordered array I get ValueError: Can't downsample if size of x or y dimension is 1. Shape: (1344, 1024, 1) Disclaimer: I know I'm doing it wrong :) I don't expect it to work. What I would prefer though, would be a more informative error message. I guess the point I would like to mention here is that imo, if the downsampling operation assumes a certain order, then one should make sure that the array is in the right axisorder. And a function to do this is there: |
@k-dominik OK, I included the axes validation before scaling which will give the nicer error. |
Barring feedback from @k-dominik, is this waiting on anything? |
No, I think it's good to go |
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.
Awesome, I really like the early failing with a super helpful error message! thx a lot!
Thanks @k-dominik and @will-moore |
Fixes #122
This uses @constantinpape code from https://github.com/ome/ome-ngff-prototypes/blob/05b55d2516941e2eaf8fa82b722cad4371f99b5f/single_image/prototypes/v03.py#L19 to validate axes.
Also adds the required dimension order to the docstring for
write_image()
andwrite_multiscale()
cc @k-dominik
To test, try the code sample below: #123 (comment) as it is, and with various shapes and valid/invalid axes. (invalid axes are not in the
tczyx
order, e.g.xyz
. This should throw anAssertionError
).NB: This previously failed to scale with unhelpful error message (since the sizeX is 1), but error should now be improved.