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

Add enums GridFormat for GMT grid format ID #3449

Merged
merged 17 commits into from
Sep 28, 2024
Merged

Add enums GridFormat for GMT grid format ID #3449

merged 17 commits into from
Sep 28, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Sep 24, 2024

Description of proposed changes

This PR is a subset of PR #3128.

This PR adds the pygmt.enums module to defined enumerations that can be used in PyGMT. The source layout is inspired by the rasterio package (https://github.com/rasterio/rasterio/blob/main/rasterio/enums.py).

Currently, only enums for GMT grid format IDs are added. They're originally defined at https://github.com/GenericMappingTools/gmt/blob/7809736ba32d87a4a96b15444419eb176c6a35f3/src/gmt_grdio.h#L70. Enums for grid registrations and gtypes are likely to be added when addressing #499 (comment).

This PR also fixes the issue originally pointed by 5888e10.

Preview: https://pygmt-dev--3449.org.readthedocs.build/en/3449/api/generated/pygmt.enums.GridFormat.html

After ea1cc79, the docs look like below:

image

After ac6239f, the docs look like below:
image

@seisman seisman marked this pull request as ready for review September 24, 2024 02:35
@seisman
Copy link
Member Author

seisman commented Sep 24, 2024

Ready for review although the documentation doesn't look correct. Maybe we need to follow https://github.com/rasterio/rasterio/blob/9953b28225db3b01193c94b1442d34b828d374aa/docs/api/rasterio.enums.rst. Will explore it later.

@seisman seisman added maintenance Boring but important stuff for the core devs needs review This PR has higher priority and needs review. labels Sep 24, 2024
@seisman seisman added this to the 0.14.0 milestone Sep 24, 2024
pygmt/enums.py Outdated Show resolved Hide resolved
@seisman seisman removed the needs review This PR has higher priority and needs review. label Sep 24, 2024
@seisman seisman marked this pull request as draft September 24, 2024 02:40
@seisman seisman changed the title Add enums for GMT grid format ID and only add the Conventions for netCDF files POC: Add enums for GMT grid format ID and only add the Conventions for netCDF files Sep 24, 2024
pygmt/enums.py Outdated Show resolved Hide resolved
pygmt/enums.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Sep 28, 2024

Still, I wouldn't say I like how the enums are documented at https://pygmt-dev--3449.org.readthedocs.build/en/3449/api/generated/pygmt.enums.GridFormat.html

@weiji14
Copy link
Member

weiji14 commented Sep 28, 2024

Still, I wouldn't say I like how the enums are documented at https://pygmt-dev--3449.org.readthedocs.build/en/3449/api/generated/pygmt.enums.GridFormat.html

Do we need to have this GridFormat enum in the API docs actually? I don't think users will use it much, it's not like you can pass it into any function/method as an argument.

I'm also ok with keeping it as is right now, and improving the style/layout later. It doesn't look too bad to me.

pygmt/enums.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Sep 28, 2024

Do we need to have this GridFormat enum in the API docs actually? I don't think users will use it much, it's not like you can pass it into any function/method as an argument.

Yes for GridFormat, but I think we need to document GridRegistration and GridType (suggested in #499 (comment)).

I'll revert the changes for docs now and explore how to make it better later.

I also prefer to splitting this PR into two smaller PRs, one for adding the GridFormat enums and one for fix the CF-Convention attribute, so that we can have two separate entries in the changelog.

These enums are defined in 'gmt_grdio.h'.
"""

UNKNOWN = 0 #: Unknown grid format
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seisman seisman changed the title POC: Add enums for GMT grid format ID and only add the Conventions for netCDF files Add enums GridFormat for GMT grid format ID Sep 28, 2024
@seisman seisman merged commit 015899b into main Sep 28, 2024
20 of 21 checks passed
@seisman seisman deleted the enums/gridid branch September 28, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants