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 Cleanup #977

Merged
merged 38 commits into from
Oct 9, 2024
Merged

Doc Cleanup #977

merged 38 commits into from
Oct 9, 2024

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Sep 27, 2024

Closes #641 #884 #743 #407

Overview

  • Refactored API reference to be better aligned with similar packages, such as xarray
  • Corrected the order of sections in the User Guide toctree
  • Updated the README
  • Updated docstrings
  • Fixed typos in some user guide sections

Rendered Documentation with Changes

@philipc2 philipc2 self-assigned this Sep 27, 2024
@philipc2
Copy link
Member Author

philipc2 commented Sep 27, 2024

@erogluorhan @rajeeja

Thoughts on removing the "Internal" and "User" API split that we've been employing since the start of the package? The only benefit of having an internal API on our documentation would be for developers.

The majority of documentations I've looked at for packages only consist of the user API.

Xarray doesn't provide an internal API.

https://docs.xarray.dev/en/stable/api.html

@rajeeja
Copy link
Contributor

rajeeja commented Sep 27, 2024

Just yesterday I had a discussion with @rljacob

Yes, we should remove that distinction. There are a few changes I have in mind for cleaning up the Docs. I will also contribute to the pr.

@philipc2
Copy link
Member Author

Just yesterday I had a discussion with @rljacob

Yes, we should remove that distinction. There are a few changes I have in mind for cleaning up the Docs. I will also contribute to the pr.

Sounds great! Feel free to discuss anything here. We could hop on a call and/or work on some together next week also.

@philipc2 philipc2 linked an issue Oct 1, 2024 that may be closed by this pull request
@erogluorhan
Copy link
Member

Sounds good to me

@philipc2 philipc2 added this to the Documentation milestone Oct 4, 2024
@philipc2 philipc2 linked an issue Oct 4, 2024 that may be closed by this pull request
@philipc2 philipc2 changed the title DRAFT: Doc Cleanup Doc Cleanup Oct 4, 2024
@philipc2
Copy link
Member Author

philipc2 commented Oct 4, 2024

@rajeeja @erogluorhan @aaronzedwick @rytam2

This should be ready for review now. There's a good chance I've probably missed something (especially in the API reference). I'm also open to any other improvements that either of you may suggest!

@philipc2 philipc2 marked this pull request as ready for review October 4, 2024 03:42
@philipc2
Copy link
Member Author

philipc2 commented Oct 4, 2024

@philipc2 philipc2 removed the request for review from erogluorhan October 8, 2024 18:24
@aaronzedwick
Copy link
Member

image
A lot of functions (as you can see in the image) have (...[,...]) or simply [...]. What is it meant to symbolize? That it takes arguments? Looks a little strange to me.

@philipc2
Copy link
Member Author

philipc2 commented Oct 9, 2024

image A lot of functions (as you can see in the image) have (...[,...]) or simply [...]. What is it meant to symbolize? That it takes arguments? Looks a little strange to me.

This is most likely due to a limitation of character length for how many parameters can be show.

image

Copy link
Collaborator

rytam2 commented Oct 9, 2024

I am thinking a bit more on the Grid section, as this section may be accessed more by folks since UXarray is developed for unstructured grids:

  • Perhaps changing the subheading like Constructor to Creating a Grid/UxDataArray/UxDataSet would be more intuitive?
  • Also would making Coordinate Attributes as a single subheading, and have spherical coordinates and cartesian coordinates a sub-subheading be a good idea?
  • Suggest moving Descriptors following right after Dimensions for more intuitive understanding in case people reading API from top-down, instead of being in between the coordinates section and connectivity.
  • Suggest adding unit for Grid.edge_node_distances and Grid.edge_face_distances

Other sections looks very neat!

@philipc2
Copy link
Member Author

philipc2 commented Oct 9, 2024

Thanks for the comments @rytam2 !

Perhaps changing the subheading like Constructor to Creating a Grid/UxDataArray/UxDataSet would be more intuitive?

The constructor isn't intended for direct construction, as that is what the class methods and API functions are for.

  • Also would making Coordinate Attributes as a single subheading, and have spherical coordinates and cartesian coordinates a sub-subheading be a good idea?

This might introduce too many sub-headers.

  • Suggest moving Descriptors following right after Dimensions for more intuitive understanding in case people reading API from top-down, instead of being in between the coordinates section and connectivity.

Right now the order is:

  • Dimensions
  • Coordinates
  • Connectivity
  • Descriptors

Do you mean moving it before the coordinates?

  • Suggest adding unit for Grid.edge_node_distances and Grid.edge_face_distances

Good idea! Will add this

@rytam2
Copy link
Collaborator

rytam2 commented Oct 9, 2024

The constructor isn't intended for direct construction, as that is what the class methods and API functions are for.

Got it; just wondering is there another term for constructor? It's fine too if there isn't.

This might introduce too many sub-headers.

Fair, I think we can keep it as is.

Do you mean moving it before the coordinates?

Yes!

@philipc2 philipc2 merged commit 8a29d96 into main Oct 9, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor "Intended" Functionality in README installation page Organize Internal API
5 participants