-
Notifications
You must be signed in to change notification settings - Fork 12
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
General update to regridding #535
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 15
Lines 1598 1605 +7
=========================================
+ Hits 1598 1605 +7
☔ View full report in Codecov by Sentry. |
This PR won't contain exposing weights, this will take a little more work and be a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jasonb5, overall the changes look good to me. I made some formatting suggestions to some docstrings to try to fit most of them in the 80 character limit.
Have you tried running make docs
in the root of the repo using the dev env to see if the docs build successfully without any errors/warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and doc changes all look good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, Jason. Thank you. I added a few minor comments (implement if you agree or ignore if they aren't needed).
|
||
>>> data_new_grid = regridder.horizontal("ts", ds) | ||
""" | ||
"""See documentation in :py:func:`xcdat.regridder.xesmf.XESMFRegridder`""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the main regrid method used in the example notebooks? Or just a function that is wrapped by something else? If it is the method users would interact with, I think we should extend the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question while going through the docs. To me, using RegridderAccessor
class might be the main method users would use? Using individual regridder class is also supported. @jasonb5 let me know if this is the case. I was going to make some doc changes anyway, so if it is okay with you, I can put together a PR to address Steve's review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pochedls @chengzhuzhang The intended usage is for users to call RegridderAccessor.horizontal
and RegridderAccessor.vertical
, these call the horizontal
and vertical
methods on the regridder clases, which are intended to stay hidden from the user.
There is a note that goes along with this.
I had originally designed the API to be used in two ways.
- A single instance where a user calls
ds.regridder.horizontal(...)
. - A multiple instance where a use might create a
regridder
from some input grid and call thehorizonal
orvertical
methods multiple times. The intention here was the weights would be calculated once and used to regrid multiple input datasets. See the example below.
regridder = xcdat.regridder.XESMFRegridder(input_grid, ...)
... = regridder.horizontal(ds1)
... = regridder.horizontal(ds2)
...
In hindsight this was not a great API design, and the reason why horizontal
and vertical
methods on the regridders will stay hidden from the user.
I have another PR coming that is going to address weights and bring this API more in line with the other xCDAT API's. This will also provide the feature of sharing weights to regrid multiple files.
weights = ...
ds.regridder.horizontal(..., weights=weights) # user defined weights
output = ds.regridder.horizontal('tas', ..., keep_weights=True)
weights = output['regrid_weights'] # keep weights to analyze or reuse.
@chengzhuzhang You can make the changes directly to my branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonb5 Thank you for the clarification and note. I will go ahead the make changes to the notebooks in your branch. The new API change that allows keeping and sharing weights sounds good to me. Though @tomvothecoder is aiming for a release next week, we can discuss tomorrow if the new change can make the new release.
Co-authored-by: Stephen Po-Chedley <[email protected]>
@chengzhuzhang Masking should work now, the mask was being dropped from the input grid. Also found a bug with regrid2 masking that is now fixed. |
The changes look good to me. Thank you so much for a quick fix! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I did another quick pass through since some files have changed.
Here are some minor updates to add a link to CDAT's regrid2
docs for legacy users or whoever is curious about it.
I also replaced the use of Union
with |
using from __future__ import annotations
. I'll start updating this in other files for a future release since it looks cleaner IMO.
Co-authored-by: Tom Vo <[email protected]>
I think all that is remaining is a test to cover the new masking code from |
Description
Update regrid documentation
Update regrid notebook
Add regridding section to FAQ
Add examples using grid property
Use grid property to drop extra dimensions e.g. time
Closes [Doc]: Add a regridding section to xCDAT FAQ #524
Closes [Doc]: Add a target grid from an existing variable to the horizontal regridding example #523
Closes [Bug]: Regridding error from differing time dimensions #497
Checklist
If applicable: