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

General update to regridding #535

Merged
merged 33 commits into from
Oct 10, 2023
Merged

General update to regridding #535

merged 33 commits into from
Oct 10, 2023

Conversation

jasonb5
Copy link
Collaborator

@jasonb5 jasonb5 commented Aug 18, 2023

Description

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (c33f0f9) 100.00% compared to head (29bdcda) 100.00%.

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     
Files Coverage Δ
xcdat/regridder/accessor.py 100.00% <100.00%> (ø)
xcdat/regridder/regrid2.py 100.00% <100.00%> (ø)
xcdat/regridder/xesmf.py 100.00% <100.00%> (ø)
xcdat/regridder/xgcm.py 100.00% <ø> (ø)

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

@tomvothecoder tomvothecoder added type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. type: enhancement New enhancement request type: docs Updates to documentation Priority: High minor A minor release, includes backwards compatible changes. labels Aug 31, 2023
@jasonb5 jasonb5 changed the title Adds grid check and updates documentation General update to regridding Sep 1, 2023
@jasonb5
Copy link
Collaborator Author

jasonb5 commented Sep 11, 2023

This PR won't contain exposing weights, this will take a little more work and be a separate PR.

@jasonb5 jasonb5 marked this pull request as ready for review September 12, 2023 17:18
docs/faqs.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@tomvothecoder tomvothecoder left a 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?

xcdat/regridder/accessor.py Show resolved Hide resolved
xcdat/regridder/xesmf.py Outdated Show resolved Hide resolved
xcdat/regridder/xesmf.py Outdated Show resolved Hide resolved
xcdat/regridder/xesmf.py Outdated Show resolved Hide resolved
xcdat/regridder/xesmf.py Outdated Show resolved Hide resolved
xcdat/regridder/xesmf.py Outdated Show resolved Hide resolved
xcdat/regridder/xgcm.py Outdated Show resolved Hide resolved
xcdat/regridder/xgcm.py Outdated Show resolved Hide resolved
xcdat/regridder/xgcm.py Outdated Show resolved Hide resolved
xcdat/regridder/xgcm.py Show resolved Hide resolved
Copy link
Collaborator

@chengzhuzhang chengzhuzhang left a 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!

Copy link
Collaborator

@pochedls pochedls left a 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).

docs/examples/regridding-horizontal.ipynb Outdated Show resolved Hide resolved
docs/examples/regridding-vertical.ipynb Outdated Show resolved Hide resolved
docs/faqs.rst Outdated Show resolved Hide resolved
xcdat/regridder/xesmf.py Outdated Show resolved Hide resolved

>>> data_new_grid = regridder.horizontal("ts", ds)
"""
"""See documentation in :py:func:`xcdat.regridder.xesmf.XESMFRegridder`"""
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

@jasonb5 jasonb5 Sep 25, 2023

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.

  1. A single instance where a user calls ds.regridder.horizontal(...).
  2. A multiple instance where a use might create a regridder from some input grid and call the horizonal or vertical 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.

Copy link
Collaborator

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 <pochedls@uw.edu>
@chengzhuzhang
Copy link
Collaborator

chengzhuzhang commented Sep 27, 2023

When doing some updates for regridding-horizontal.ipynb, I won't be able to reproduce the masking case. In my notebook, the tas field is not masked:
Screen Shot 2023-09-26 at 4 37 17 PM
The expected figure as follows:
Screen Shot 2023-09-26 at 4 37 43 PM

It could be an environment issue, i used the conda recipe on main branch to install. I'm on a M1 macbook, so I used CONDA_SUBDIR=osx-64 to create my conda enviroment.

Update
If i use xesmf.Regridder directly then the data masking works okay. It led me to think instead of a environment problem, it might be some recent change that changed the regridding behavior...

below uses xesmf.Regridder:
Screen Shot 2023-09-26 at 8 10 37 PM

@tomvothecoder tomvothecoder added this to the v0.6.0 milestone Sep 27, 2023
@jasonb5
Copy link
Collaborator Author

jasonb5 commented Sep 28, 2023

@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.

@chengzhuzhang
Copy link
Collaborator

The changes look good to me. Thank you so much for a quick fix!

Copy link
Collaborator

@tomvothecoder tomvothecoder left a 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.

docs/faqs.rst Outdated Show resolved Hide resolved
docs/faqs.rst Show resolved Hide resolved
xcdat/regridder/accessor.py Show resolved Hide resolved
xcdat/regridder/accessor.py Outdated Show resolved Hide resolved
xcdat/regridder/accessor.py Outdated Show resolved Hide resolved
Co-authored-by: Tom Vo <tomvothecoder@gmail.com>
@tomvothecoder
Copy link
Collaborator

I think all that is remaining is a test to cover the new masking code from a0ad524 (#535) then we should be ready to merge?

@jasonb5 jasonb5 merged commit bb1fff6 into xCDAT:main Oct 10, 2023
9 checks passed
@jasonb5 jasonb5 deleted the fix_regrid_docs branch October 10, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor A minor release, includes backwards compatible changes. type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. type: docs Updates to documentation type: enhancement New enhancement request
Projects
Status: Done
4 participants