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

Implement new "most common" regridder. #46

Merged
merged 14 commits into from
Sep 25, 2024

Conversation

BSchilperoort
Copy link
Contributor

@BSchilperoort BSchilperoort commented Sep 10, 2024

This implements the method discussed in xarray-contrib/flox#263

Also added regrid.stat to implement other aggregations from flox.

Todo:

  • add tests for regrid.stat

@BSchilperoort
Copy link
Contributor Author

@dcherian I don't seem to be a maintainer on this repo. Perhaps due to the repository not being linked to the team? See https://github.com/orgs/xarray-contrib/teams/xarray-regrid-devs/repositories

@dcherian
Copy link

I changed it here: https://github.com/xarray-contrib/xarray-regrid/settings/access Should work now?

@slevang
Copy link
Collaborator

slevang commented Sep 24, 2024

This looks like a nice improvement, is it close to ready? It would be great to push a release that includes this plus #45 and #49.

@BSchilperoort
Copy link
Contributor Author

This looks like a nice improvement, is it close to ready? It would be great to push a release that includes this plus #45 and #49.

I will try to have it ready by the end of this week. If I don't manage we can make a new release that includes the improvements you worked on.

@BSchilperoort
Copy link
Contributor Author

@dcherian the routines seem very fast and efficient now. Didn't see too many memory issues, and especially the statistical reduction method is very fast. Thanks for your help!

@BSchilperoort
Copy link
Contributor Author

PR is basically ready, except for the padding issue with the statistic based reduction methods. The results of the regridding and edge cases are also not fully tested yet, but that can probably be added later.

Copy link
Collaborator

@slevang slevang left a comment

Choose a reason for hiding this comment

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

Looks good! Padding issue should be fixed.

assert formatted.longitude[0] == -1.5
assert formatted.longitude[-1] == 361.5
# And preserve integer dtypes
assert formatted.data.dtype == np.int64
Copy link
Collaborator

@slevang slevang Sep 25, 2024

Choose a reason for hiding this comment

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

I couldn't find any unwanted dtype conversion with longitude padding, because there we're doing a .pad using the existing data. The latitude padding was converting to float, because I set that up to concat the .mean() of the last latitude rows as an extra point. Added a test to check this.

I haven't tested the behavior at the poles with the statistical aggregations to know whether we actually do want some padding there in certain cases. If we do, we would need some new logic to determine what to pad with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah not sure what went wrong. I could not run the demo notebook with the landcover data, as that was unint8. Everything now seems to run OK 😕 .

I haven't tested the behavior at the poles with the statistical aggregations to know whether we actually do want some padding there in certain cases. If we do, we would need some new logic to determine what to pad with.

I think we would not want to do any padding there. The zonal statistics methods really are all about groupby/binning reductions.

src/xarray_regrid/utils.py Show resolved Hide resolved
src/xarray_regrid/utils.py Show resolved Hide resolved
@BSchilperoort BSchilperoort merged commit d8b7b23 into xarray-contrib:main Sep 25, 2024
11 checks passed
@BSchilperoort BSchilperoort deleted the new-most-common branch September 25, 2024 14:58
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