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

Add destaggering functionality #93

Merged
merged 14 commits into from
Sep 16, 2022
Merged

Conversation

jthielen
Copy link
Collaborator

@jthielen jthielen commented Sep 9, 2022

Change Summary

This is an updated version of #37 to handle de-staggering in all three dimensions and in a (hopefully, still needs to be tested) dask-friendly way.

Currently a draft PR to get eyes on my implementation right away; will be working on tests, documentation updates, and cleaning up the type hints later today.

Related issue number

Closes #35

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@jthielen jthielen added the enhancement New feature or request label Sep 9, 2022
@jthielen jthielen added this to the v0.0.2 milestone Sep 9, 2022
@jthielen jthielen mentioned this pull request Sep 9, 2022
3 tasks
@lpilz lpilz mentioned this pull request Sep 13, 2022
1 task
@jthielen jthielen force-pushed the add-destagger branch 3 times, most recently from 35f6fa6 to a194f29 Compare September 14, 2022 16:23
@lpilz
Copy link
Collaborator

lpilz commented Sep 14, 2022

I think it would be nice to support destaggering multiple dimensions at once. Couldn't we just recursively call _destagger_variable for a single dim?

@jthielen
Copy link
Collaborator Author

jthielen commented Sep 14, 2022

I think it would be nice to support destaggering multiple dimensions at once. Couldn't we just recursively call _destagger_variable for a single dim?

It would require a little bit of refactoring of the automatic staggered dim handling and a bit more internal complexity on the accessor methods, but if this is a use case we wanted to support, I could certainly make those changes! That being said, I don't believe I've seen this situation occur in practice. Do you have a sample dataset with such a variable with multiple staggered dimensions?

@lpilz
Copy link
Collaborator

lpilz commented Sep 14, 2022

You're right, I was originally thinking about maybe finding some variables staggered in z and one of the x or y dimensions, but it seems that this doesn't occur (at least in output data). So nevermind! :D

@jthielen jthielen marked this pull request as ready for review September 15, 2022 17:29
@jthielen jthielen requested a review from a team September 15, 2022 17:30
@jthielen jthielen mentioned this pull request Sep 15, 2022
Copy link
Collaborator

@lpilz lpilz left a comment

Choose a reason for hiding this comment

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

Very nice work 👍 I have just a couple of small questions.

tests/test_accessors.py Show resolved Hide resolved
xwrf/destagger.py Outdated Show resolved Hide resolved
xwrf/accessors.py Outdated Show resolved Hide resolved
xwrf/destagger.py Outdated Show resolved Hide resolved
@jthielen
Copy link
Collaborator Author

While this doesn't add direct tests with dask (deferring to #60 and a later release...perhaps v0.1?), with some some cursory exploration it looks like everything works out for #52 (comment).

jthielen and others added 2 commits September 15, 2022 12:04
Co-authored-by: Lukas Pilz <mail@lukaspilz.de>
Co-authored-by: Lukas Pilz <mail@lukaspilz.de>
xwrf/accessors.py Outdated Show resolved Hide resolved
jthielen and others added 3 commits September 15, 2022 20:57
@jthielen
Copy link
Collaborator Author

Thank you for the feedback @lpilz and @andersy005! I believe all the issues raised have been addressed. With an approving review from someone on @xarray-contrib/xwrf-devs, this should be good to merge.

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Great work @jthielen !!

@jthielen jthielen merged commit 290c2e7 into xarray-contrib:main Sep 16, 2022
@lpilz
Copy link
Collaborator

lpilz commented Sep 16, 2022

Awesome work 🚀 Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: destagger function enabled for xarray
5 participants