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.

Sign up for GitHub

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

Jump to bottom

Use new *Coarsen.reduce functionality for coarsening with custom reductions #1048

Closed
spencerkclark opened this issue Feb 26, 2021 · 6 comments
Closed

Use new *Coarsen.reduce functionality for coarsening with custom reductions #1048

spencerkclark opened this issue Feb 26, 2021 · 6 comments
Labels
refactor

Comments

@spencerkclark
Copy link
Member

Xarray 0.17.0 adds a .reduce method to DataArrayCoarsen and DatasetCoarsen, which natively enables coarsening DataArrays and Datasets with custom reduction functions. This obviates some functions in vcm, namely xarray_block_reduce and horizontal_block_reduce (as well as some associated private functions). Some care will be needed to maintain dask compatibility for some operations, but it should hopefully be possible without too much trouble.

The main motivation for doing this is that it should allow us to drop the dependency on scikit-image.
@spencerkclark spencerkclark mentioned this issue Feb 26, 2021
Merged
@nbren12
Copy link
Contributor

nbren12 commented Feb 26, 2021

This would also solve the cftime-xarray intergration as well right? (Related to #986 )

@spencerkclark
Copy link
Member Author

Yes, exactly (sorry if my comment was vague earlier).

@nbren12
Copy link
Contributor

nbren12 commented Feb 26, 2021

No it was clear, I just didn't see it.

@nbren12 nbren12 mentioned this issue Mar 17, 2021
Merged
nbren12 added a commit that referenced this issue Mar 18, 2021
@nbren12
Vendorize skimage (#1091)
2ef7137 
Scikit-image is a difficult dependency to manage (especially since it makes strong assumptions about pooch). Even an import of scikit-image mutates the users ~/.cache folder, which can lead to hard to debug permissions errors and other issues. We work around this via careful dependency pinning, but I still frequently run into this problem when e.g. updating versions. We only use two small functions from scikit-image, so it's easy to vendorize them in this repo.

We can reimplement this using `xarray.coarsen` at another time (see #1048).

Added public API:
- vcm.testing.checksum_dataarray/checksum_dataarray_mapping/regression_data

Significant internal changes:
- rewired the coarening tests to use regression checksums instead.

Requirement changes:
- removed scikit-image
- I would've liked to remove pooch too, but metpy uses it too, albeit only in one module that we probably don't import.
@spencerkclark
Copy link
Member Author

@mcgibbon given that @nbren12 vendored the required functionality from scikit-image (removing our dependency on it) I consider this refactor to be low priority (if necessary at all). I would be fine with closing this if we'd like to clean up the issue tracker.

@mcgibbon
Copy link
Contributor

If the refactor isn't necessary I'd say to close it to keep things clean, if you think this is something I should spend time on in let's say the next two months then it can stay open. If not I'd think it isn't likely to ever get done.

@spencerkclark
Copy link
Member Author

Yeah, I don't think this is worth spending your time on. Let's close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nbren12 @spencerkclark @mcgibbon