-
Notifications
You must be signed in to change notification settings - Fork 5
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
BUG: Fix zero-sized slicing and improve test coverage #49
Merged
tbirdso
merged 5 commits into
InsightSoftwareConsortium:master
from
tbirdso:zero-valued-slicing-fix
Aug 13, 2023
Merged
BUG: Fix zero-sized slicing and improve test coverage #49
tbirdso
merged 5 commits into
InsightSoftwareConsortium:master
from
tbirdso:zero-valued-slicing-fix
Aug 13, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes an issue where Tensorstore would allow reading a slice of size zero into an empty array buffer, with the result being an array of all zeros.
Adds more verbose printouts to help with debugging and logging
Adds tests to read 2D frames from a contrived 2D+time OME-Zarr test image. Tests include output comparison in MetaImage format to capture metadata and voxel equivalence.
Adds baseline MetaImage comparisons for HTTP tests at various resolution levels. Provides coverage to validate both metadata and voxel agreement with baseline.
thewtex
approved these changes
Aug 11, 2023
dzenanz
reviewed
Aug 11, 2023
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.
Mostly looks good.
dzenanz
approved these changes
Aug 11, 2023
I am able to reproduce test failures on my Linux machine. Will investigate. |
Removes metadata validation from HTTP tests. Both metadata validation and image buffer validation are now covered under baseline image comparison after the test body completes.
tbirdso
force-pushed
the
zero-valued-slicing-fix
branch
from
August 13, 2023 16:34
50b55d2
to
3d6984e
Compare
Tests are passing after addressing a small test build warning. (had incorrectly marked a test function with Moving forward with merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses issue where time and channel slice sizes were set to zero instead of one. The result of this issue was such that reading from stores with time or channel axes returned correct metadata but all-zero-valued image buffers.
This issue slipped by because I was testing for metadata and voxel array shape correctness, but not the actual content of the images that were produced. I have overhauled testing to cover this case and prevent similar regressions in the future:
t,y,x
zarr store with two time frames and validate that each frame has correct metadata and image content.Also adds optional debugging output to
OMEZarrNGFFImageIO
to help decipher ITK/tensorstore mismatch issues in the future.