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

Update regridding API naming convention #510

Closed
wants to merge 7 commits into from

Conversation

jasonb5
Copy link
Collaborator

@jasonb5 jasonb5 commented Jun 19, 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-commenter
Copy link

codecov-commenter commented Jun 19, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.14 ⚠️

Comparison is base (870b334) 100.00% compared to head (d9b19af) 99.86%.

❗ Current head d9b19af differs from pull request most recent head b10940a. Consider uploading reports for the commit b10940a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #510      +/-   ##
===========================================
- Coverage   100.00%   99.86%   -0.14%     
===========================================
  Files           15       15              
  Lines         1562     1524      -38     
===========================================
- Hits          1562     1522      -40     
- Misses           0        2       +2     
Impacted Files Coverage Δ
xcdat/regridder/xesmf.py 100.00% <ø> (ø)
xcdat/regridder/xgcm.py 100.00% <ø> (ø)
xcdat/regridder/accessor.py 96.96% <77.77%> (-3.04%) ⬇️
xcdat/regridder/regrid2.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tomvothecoder
Copy link
Collaborator

Thanks for this PR. Feel free to assign reviewers whenever PRs are ready to get things rolling.

I'll do an initial review.

@tomvothecoder tomvothecoder self-requested a review June 20, 2023 17:38
@tomvothecoder tomvothecoder added the type: enhancement New enhancement request label Jun 20, 2023
@tomvothecoder
Copy link
Collaborator

I don't think this PR is ready for review so I'll hold off on reviewing

@jasonb5
Copy link
Collaborator Author

jasonb5 commented Jun 21, 2023

@tomvothecoder This is ready for review.
@chengzhuzhang @lee1043 @pochedls Feel free to invite other reviewers. I'm hoping these changes will improve the visibility of the regridder documentation. Let me know if anything is still unclear and can be improved, thank you!

@tomvothecoder tomvothecoder changed the title Update regridding api Update regridding API naming convention Jun 21, 2023
@jasonb5 jasonb5 marked this pull request as draft June 21, 2023 17:40
@jasonb5
Copy link
Collaborator Author

jasonb5 commented Jun 21, 2023

Setting this as a draft as it depends on #507. I'll need to make some additional updates to documentation after it's merged.

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Jun 21, 2023

Hey @jasonb5, let's push this PR back to give more time for discussion since it is not a crucial for v0.6.0. @chengzhuzhang and @lee1043 are in favor of keeping the .horizontal() wrapper that encapsulates all of the tools through the tool arg.

I tried to summarize our discussion at the meeting below:

Why this PR was kickstarted (#501)

Reasons to keep .horizontal() wrapper

  • If users are really interested in the regridder options, they will read the docstring and go to the individual API pages
  • It gives more flexibility in usage and testing since the tool argument just needs to be swapped vs. importing individual APIs
  • xesmf is a widely used tool and should be the default option
    • regrid2 is mainly for CDAT users who need to reproduce the same results using xCDAT

@tomvothecoder
Copy link
Collaborator

I came across this style of reusable docstrings in xarray. It might be useful if we want to consolidate and reuse docstrings:

https://github.com/pydata/xarray/blob/main/xarray/core/weighted.py#L25-L52
https://github.com/pydata/xarray/blob/1146c5a3cdb15d402e95153c3ea68826b158d953/xarray/core/weighted.py#L546-L569

@jasonb5
Copy link
Collaborator Author

jasonb5 commented Sep 25, 2023

Closing as we've decided to stick with generic horizontal and vertical methods. PR #535 addresses the documentation changes in this PR.

@jasonb5 jasonb5 closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New enhancement request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Doc]: Consider updating the listed APIs for horizontal regridding
3 participants