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

[Doc]: Consider updating the listed APIs for horizontal regridding #501

Closed
tomvothecoder opened this issue Jun 8, 2023 · 7 comments
Closed
Assignees
Labels
type: docs Updates to documentation

Comments

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Jun 8, 2023

Describe your documentation update

After revisiting the horizontal regridding APIs we have listed in the API Reference page, I think listing all of these options can be confusing for the user.

  1. Python classes
  2. General accessor method
  3. Individual accessor method by tool

All of these methods achieve the same functions, but go about it in a slightly different way.
This is the callstack with xesmf: horizontal(...tool="xesmf") -> horizonal_xesmf() -> XESMFRegridder().

Solution

I propose we just keep "2. General accessor method" listed in our documentation. I foresee most people using that API since it is the most straightforward to use and most flexible (you can specify tool="regrid2" or tool="xesmf").

@tomvothecoder tomvothecoder added the type: docs Updates to documentation label Jun 8, 2023
@tomvothecoder
Copy link
Collaborator Author

Any thoughts @xCDAT/core-developers?

@pochedls
Copy link
Collaborator

pochedls commented Jun 8, 2023

I haven't investigated this enough to weigh in on what is right, but I do find the regridder documentation a little confusing (as mentioned here). It seems like 2 has the correct documentation.

One note regarding these references:

Find options at xcdat.regridder.xesmf.XESMFRegridder()

I think this is probably the best way to note/refer to options specific to xESMF / regrid2, but if there was a way of putting it within the documentation here that could be helpful.

@jasonb5
Copy link
Collaborator

jasonb5 commented Jun 8, 2023

Describe your documentation update

After revisiting the horizontal regridding APIs we have listed in the API Reference page, I think listing all of these options can be confusing for the user.

1. Python classes
   
   * https://xcdat.readthedocs.io/en/stable/generated/xcdat.regridder.regrid2.Regrid2Regridder.html
   * https://xcdat.readthedocs.io/en/stable/generated/xcdat.regridder.xesmf.XESMFRegridder.html

These are purely there for documentation, that's the only way to make a link to reference the individual regridder options.

2. General accessor method
   
   * https://xcdat.readthedocs.io/en/stable/generated/xarray.Dataset.regridder.horizontal.html

3. Individual accessor method by tool
   
   * https://xcdat.readthedocs.io/en/stable/generated/xarray.Dataset.regridder.horizontal_xesmf.html
   * https://xcdat.readthedocs.io/en/stable/generated/xarray.Dataset.regridder.horizontal_regrid2.html

All of these methods achieve the same functions, but go about it in a slightly different way. This is the callstack with xesmf: horizontal(...tool="xesmf") -> horizonal_xesmf() -> XESMFRegridder().

Solution

I propose we just keep "2. General accessor method" listed in our documentation. I foresee most people using that API since it is the most straightforward to use and most flexible (you can specify tool="regrid2" or tool="xesmf").

I'd be fine removing horizontal_xesmf and horizontal_regrid2, they really only save a single argument.


I haven't investigated this enough to weigh in on what is right, but I do find the regridder documentation a little confusing (as mentioned here). It seems like 2 has the correct documentation.

One note regarding these references:

Find options at xcdat.regridder.xesmf.XESMFRegridder()

I think this is probably the best way to note/refer to options specific to xESMF / regrid2, but if there was a way of putting it within the documentation here that could be helpful.

There's not really a good way to place the documentation here. I could try some regex magic and combine the regrid2 and xesmf docstring into a single one but it brings up another issue. If we ever add more regridders, we could end up with one very large docstring.

I'll also make another pass on the documentation with the insight from this issue.

@tomvothecoder
Copy link
Collaborator Author

These are purely there for documentation, that's the only way to make a link to reference the individual regridder options.

You're right, I forgot you need to list the accessor class to list its methods and attributes.

I'd be fine removing horizontal_xesmf and horizontal_regrid2, they really only save a single argument.

Yeah, I noticed people mostly using horizontal() in their code/notebooks. Those underlying APIs include xesmf or regrid2 in the name of the method, which almost as many characters tool='xesmf'.

@jasonb5
Copy link
Collaborator

jasonb5 commented Jun 9, 2023

After some more thought we could solve the documentation issue by getting rid of horizontal() and vertical() and just have regrid2(), xesmf(), and xgcm(). We could group them together under horizontal and vertical in the documentation since users will probably learn about them there.

@jasonb5 jasonb5 self-assigned this Jun 9, 2023
@jasonb5
Copy link
Collaborator

jasonb5 commented Jun 9, 2023

After the last meeting, the plan is to deprecate the current regridding accessor functions and remove them in a few release cycles.

  • horizontal()
  • vertical()

The horizontal_xesmf() and horizontal_regrid2() will remain and vertical_xgcm() will be added. Retaining the horizontal and vertical prefix will allow the possibility for a regridder to implement both vertical and horizontal.

The documentation will be moved to these methods so there will no longer be a link from the accessor methods to class __init__ methods e.g. accessor method -> class init. Once this is done https://xcdat.readthedocs.io/en/stable/generated/xcdat.regridder.regrid2.Regrid2Regridder.html and https://xcdat.readthedocs.io/en/stable/generated/xcdat.regridder.xesmf.XESMFRegridder.html can be removed. This will drastically clean up the regridding documentation and make the individual regridders documentation easier to access.

I will also do a pass on all the regridding documentation to update anything requiring clarification.

@jasonb5
Copy link
Collaborator

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 as completed Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs Updates to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants