Conversation
for more information, see https://pre-commit.ci
…r-py into clean-scalers
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…r-py into clean-scalers
for more information, see https://pre-commit.ci
…r-py into clean-scalers
for more information, see https://pre-commit.ci
…r-py into clean-scalers
|
I see that some of the failing tests are for writing v0.1 - v0.3 of OME-Zarr. I think we could drop support for these and focus on |
|
Hi @will-moore thanks again for the feedback. I have refactored the scale test yet a bit more. I figured it would be better to use For instance, if Edit: I think it would have been nice to parse the scale factors in a central place (i.e., inside |
|
@will-moore all good points. I added another test function that passed only different instances of I also moved all the deprecation warnings into a single place - them being still there was probably the result of some botched merge conflict. |
|
Tests look good, thanks. Just the one minor comment above. In testing this, the only issue that I'd like to re-affirm (discuss again) is the default z-downsampling behaviour. It's a tricky decision which we may want to consult about more widely... Is 3D downsampling considered "more correct" than 2D downsampling by the community (and our users) in general? It's a bit less verbose to specify 2D (if the default is 3D) than vice-versa. Making 3D downsampling the default is more of a breaking change. #262 is a long-standing request for 3D downsampling, but I guess it doesn't need to be the default. |
|
@will-moore I think my overall stance would be that I would downsample in z by default. When I first used ome-zarr-py it seemed a bit arbitrary why the z-dimension would specifically be excluded from downsampling by default. But this PR is not about this matter, imho. It only makes it possible to apply equal/custom downsampling along all axes but you still have to make that choice yourself as a user. The reason for this PR is more to consolidate code (with #531 and #527 coming up in the wake of that) to make the repo ready for some bigger architectural changes later on (i.e., RFC5 and #515). For the sake of going forward I would keep the default as it is (do not downsample in z) and then send a smaller PR that would change the default behavior which can then contain the discussion around #262 that may or may not evolve around it. |
Title. Since the scaling functionality is currently scattered across a few places (
Scalerclass anddask_utils), I discussed with @will-moore that it could be a could thing to deprecate one of them.In this PR, I
Scalerclassscale.build_pyramid)chunkskwarg fromwrite_image,write_labels,write_multiscale_imageandwrite_multiscale_labelsTODOs:
build_pyramidsomewhere or make private