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

raise error for invalid scale factors #73

Merged
merged 8 commits into from
Feb 9, 2023

Conversation

giovp
Copy link
Contributor

@giovp giovp commented Feb 2, 2023

cross ref from here scverse/spatialdata#110

@thewtex here a ValueError is raised but please let me know if you think a logging or just a warning should be raised instead.

I tried to make dask fail with invalid chunk sizes as described here: scverse/spatialdata#110 (comment) I think I understood what you mean but I didn't manage to make a reprex with couple of resizing methods (Methods.XARRAY_COARSEN and Methods.DASK_IMAGE_NEAREST). Basically even if I pass larger-than-axes chunks the method would still bound to the max value for that dimension, I'll try a bit more later. On a separate note, should this check happen at inferred chunk sizes from the passed chunks and 1image.shape` or should it happen after the resizing already completed (before returning the DataTree). Thank you!

thewtex
thewtex previously approved these changes Feb 5, 2023
Copy link
Contributor

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

💯

@thewtex thewtex dismissed their stale review February 5, 2023 19:06

CI failing

@thewtex
Copy link
Contributor

thewtex commented Feb 5, 2023

@giovp thank you for the contribution! 👏

Overall looks good, CI is failing due handling of the dimensions (it may help to use the dict identifier for dimensions).

Thanks for taking a look at the chunk size issue. Yes, we can look at the chunk size issue as a follow-up. It may be at reduced scales that the chunk sizes need to be reduced to be at most the shape of the reduced array (I think this could be done during multiscale generation automatically when required).

@giovp
Copy link
Contributor Author

giovp commented Feb 6, 2023

@thewtex I believe the test now fails cause time t is also included in the check against scale factors, with error:

FAILED test/test_ngff_validation.py::test_t_z_y_x_c_valid_ngff - ValueError: Scale factor 4 is incompatible with image shape (2, 32, 32, 16, 3) along dimension t.

is this desirable or shall I exclude t the same way channels c are excluded?

@thewtex
Copy link
Contributor

thewtex commented Feb 7, 2023

exclude t the same way channels c are excluded?

yes, please 🙏

@giovp giovp requested a review from thewtex February 7, 2023 14:45
Copy link
Contributor

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

💯

@thewtex thewtex merged commit 77c0120 into spatial-image:main Feb 9, 2023
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