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

Allow to change downsample function #358

Merged
merged 7 commits into from
Apr 24, 2024

Conversation

nhatnm52
Copy link
Contributor

@nhatnm52 nhatnm52 commented Mar 4, 2024

  • Define property func to get downsample function.
  • Use scaler.func instead of scaler.nearest in function _create_mip.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.42%. Comparing base (a812416) to head (93428aa).
Report is 2 commits behind head on master.

Files Patch % Lines
ome_zarr/scale.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   85.06%   85.42%   +0.36%     
==========================================
  Files          13       13              
  Lines        1533     1537       +4     
==========================================
+ Hits         1304     1313       +9     
+ Misses        229      224       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@will-moore
Copy link
Member

Hi @nhatnm52 and thanks for the contribution.
Is this a bug-fix or a new feature?
Either way, it would be great if you could add a test to https://github.com/ome/ome-zarr-py/blob/master/tests/test_scaler.py that requires your change, both to help us understand the change and make sure it isn't lost.
Thanks,
Will

@nhatnm52 nhatnm52 force-pushed the write-with-more-downsample-methods branch from c034ccd to eacdca7 Compare March 14, 2024 15:01
@nhatnm52
Copy link
Contributor Author

Hi Will, I have a private project and I need to write an image into a pyramid image with local_mean method, your _create_mip function only allows nearest so I use func property to allow this. I set up a test for 2 cases of Scaler class. I hope this change will be helpful.
Nhat Nguyen.

@will-moore
Copy link
Member

Hi, thanks for the clarification.
Looking at the tests, it occurred to me that for several tests it's not really checking that the correct downsampling method was chosen because we're only checking the dimensions of the downsampled data, not the pixels.

E.g. if I edit your scale.py to always use `nearest' like this:

@property
    def func(self) -> Callable[[np.ndarray], List[np.ndarray]]:
        """Get downsample function."""
        func = getattr(self, "nearest", None)

the only tests that fail are the def test_scale_dask() since that's the only one that checks the pixels.

Instead of adding the extra directly parameter to every test, you could instead do something like this: (you don't need to do this for all tests, especially the ones that are marked as failing. Maybe just do it for test_local_mean and test_scale_dask

e.g. def test_local_mean()

        downscaled = scaler.local_mean(data)
        scaler.method = "local_mean"
        assert np.array_equal(downscaled, scaler.func(data))
        self.check_downscaled(downscaled, shape)

I also noticed that our tests don't actually test scaler.scale() anywhere, but that's not a blocker for this PR.

Cheers!

@nhatnm52
Copy link
Contributor Author

Hi, I rolled back pytest.fixture as original and added testing by pixel-wise for local_mean, nearest and scale_dask, I skipped test cases using skip or xfail flags. This test will ensure that the func property performs its function.

Have a nice working day.

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

LGTM thanks.

@joshmoore joshmoore merged commit e33f736 into ome:master Apr 24, 2024
20 checks passed
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.

3 participants