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

Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" to load the "free-air anomaly uncertainty" dataset #3727

Merged
merged 33 commits into from
Dec 28, 2024

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Dec 26, 2024

Description of proposed changes

Add the parameter uncertainty to the function load_earth_free_air_anomaly to load the dataset "IGPP Earth Free-Air Anomaly Uncertainty"

Adresses #2431

Preview: https://pygmt-dev--3727.org.readthedocs.build/en/3727/api/generated/pygmt.datasets.load_earth_free_air_anomaly.html

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash command is:

  • /format: automatically format and lint the code

@yvonnefroehlich yvonnefroehlich added the feature Brand new feature label Dec 26, 2024
@yvonnefroehlich yvonnefroehlich self-assigned this Dec 26, 2024
@yvonnefroehlich yvonnefroehlich changed the title Remote datasets: Add the "IGPP Earth Free-Air Anomaly Errors" dataset to "load_earth_free_air_anomaly" WIP: Remote datasets: Add the "IGPP Earth Free-Air Anomaly Errors" dataset to "load_earth_free_air_anomaly" Dec 26, 2024
@yvonnefroehlich yvonnefroehlich marked this pull request as draft December 26, 2024 21:16
@@ -20,36 +21,42 @@ def load_earth_free_air_anomaly(
] = "01d",
region: Sequence[float] | str | None = None,
registration: Literal["gridline", "pixel", None] = None,
data_source: Literal["faa", "faaerror"] = "faa",
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the parameter name data_source here, since these two datasets acutally comes from the same source.

Maybe uncertainty=True to use the faaerror data?

pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
yvonnefroehlich and others added 2 commits December 27, 2024 12:00
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
yvonnefroehlich and others added 3 commits December 27, 2024 12:02
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
@yvonnefroehlich yvonnefroehlich changed the title WIP: Remote datasets: Add the "IGPP Earth Free-Air Anomaly Errors" dataset to "load_earth_free_air_anomaly" Remote datasets: Add the "IGPP Earth Free-Air Anomaly Errors" dataset to "load_earth_free_air_anomaly" Dec 27, 2024
@yvonnefroehlich yvonnefroehlich marked this pull request as ready for review December 27, 2024 11:10
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good to me except minor suggestion on docstrings.

pygmt/datasets/earth_free_air_anomaly.py Show resolved Hide resolved

IGPP Earth free-air anomaly dataset.
* - IGPP Earth Free-Air Anomaly
- IGPP Earth Free-Air Anomaly Errors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- IGPP Earth Free-Air Anomaly Errors
- IGPP Earth Free-Air Anomaly Uncertainty

Copy link
Member Author

@yvonnefroehlich yvonnefroehlich Dec 27, 2024

Choose a reason for hiding this comment

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

Hm. I used the name for the upstream docs at https://www.generic-mapping-tools.org/remote-datasets/earth-faaerror.html. But I was wondering, if "uncertainty" is better, as it is used in the docstrings. Maybe we should also update the upstream docs; I can include this in PR GenericMappingTools/remote-datasets#128 if approved.

Copy link
Member

Choose a reason for hiding this comment

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

The GMT remote-dataset docs is inconsistenly using "error" and "uncertainty", while the officiail README file (https://topex.ucsd.edu/pub/global_grav_1min/README_V32.txt) uses "uncertainty":

grav_error_32.1.nc - uncertainty in gravity anomaly and sea surface slope, milligal or microradian, netcdf, lon-lat coordinates

Copy link
Member Author

Choose a reason for hiding this comment

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

Made commit GenericMappingTools/remote-datasets@5aa5e58 for changing this in the GMT remote-dataset docs.

But we stick to "error" in the GMT dataset name faaerror ?

Copy link
Member

Choose a reason for hiding this comment

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

I think faaerror is fine.

pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Dec 27, 2024

BTW, maybe change the PR title to "load_earth_free_air_anomaly: Add the 'uncertainty' parameter to return free-air anomaly errors".

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Dec 27, 2024
@seisman seisman mentioned this pull request Dec 27, 2024
49 tasks
@yvonnefroehlich yvonnefroehlich changed the title Remote datasets: Add the "IGPP Earth Free-Air Anomaly Errors" dataset to "load_earth_free_air_anomaly" Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" to return the free-air anomaly uncertanity dataset Dec 27, 2024
@yvonnefroehlich yvonnefroehlich changed the title Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" to return the free-air anomaly uncertanity dataset Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" for free-air anomaly uncertanity dataset Dec 27, 2024
@yvonnefroehlich yvonnefroehlich changed the title Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" for free-air anomaly uncertanity dataset Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" for "free-air anomaly uncertainty" dataset Dec 27, 2024
@yvonnefroehlich yvonnefroehlich changed the title Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" for "free-air anomaly uncertainty" dataset Remote datasets: Add "uncertainty" parameter to "load_earth_free_air_anomaly" to load the "free-air anomaly uncertainty" dataset Dec 27, 2024
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 28, 2024
@seisman seisman merged commit 230ff31 into main Dec 28, 2024
21 of 22 checks passed
@seisman seisman deleted the add-remote-earth-faaerror branch December 28, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants