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 load_earth_deflection to load "IGPP Earth east-west and north-south deflection" datasets #3728

Merged
merged 22 commits into from
Dec 30, 2024

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Dec 27, 2024

Description of proposed changes

Add function load_earth_deflection to download the "IGPP Earth east-west and north-south deflection" datasets.

Adresses #2431

Preview: https://pygmt-dev--3728.org.readthedocs.build/en/3728/api/generated/pygmt.datasets.load_earth_deflection.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 self-assigned this Dec 27, 2024
@yvonnefroehlich yvonnefroehlich added the feature Brand new feature label Dec 27, 2024
@yvonnefroehlich yvonnefroehlich added this to the 0.14.0 milestone Dec 27, 2024
@yvonnefroehlich yvonnefroehlich marked this pull request as draft December 27, 2024 20:13
@yvonnefroehlich yvonnefroehlich changed the title Remote datasets: Add "IGPP Earth east-west and south-north deflection datases WIP: Remote datasets: Add "IGPP Earth east-west and south-north deflection datases Dec 27, 2024
] = "01d",
region: Sequence[float] | str | None = None,
registration: Literal["gridline", "pixel", None] = None,
direction: Literal["edefl", "ndefl"] = "edefl",
Copy link
Member Author

Choose a reason for hiding this comment

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

If people have a more suitable name for this new parameter, feel free to make suggestions. I just picked the "east-west deflection" dataset as default.

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 the parameter is OK, but for the arguments, "edefl"/"ndefl" is not readable. Maybe "e"/"n" or "east"/"north" or "east_west"/"north_south", but I have to admit that I have no idea about the physical meaning of this dataset

Copy link
Member Author

Choose a reason for hiding this comment

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

See comment #3728 (comment).

] = "01d",
region: Sequence[float] | str | None = None,
registration: Literal["gridline", "pixel", None] = None,
direction: Literal["east_west", "south_north"] = "east_west",
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 it should be "north_south" rather than "south_north", mainly because:

  1. East and North are pointing to the positive values
  2. The GMT dataset names are "edefl" and "ndefl".
  3. The original source files are named as "east_xxx" and "north_xxx"
Suggested change
direction: Literal["east_west", "south_north"] = "east_west",
direction: Literal["east_west", "north_south"] = "east_west",

I think we should also fix "south-north" to "north-south" in the remote-dataset page.

Copy link
Member Author

@yvonnefroehlich yvonnefroehlich Dec 28, 2024

Choose a reason for hiding this comment

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

Yes, I am also a bit wondering about the dataset names and coordinate system axes directions, and currently looking at in the literature about this naming.

Copy link
Member Author

@yvonnefroehlich yvonnefroehlich Dec 28, 2024

Choose a reason for hiding this comment

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

Just saw that the remote dataset docs are actually not consistent, and both "south-north"
(title and image describtion https://www.generic-mapping-tools.org/remote-datasets/earth-ndefl.html#igpp-earth-south-north-deflections) and "north-south" (in section usage https://www.generic-mapping-tools.org/remote-datasets/earth-ndefl.html#usage) are used.

Wikipedia says is's "east-west" and "north-south" (https://en.wikipedia.org/wiki/Vertical_deflection) and here https://geodesy.noaa.gov/GEOID/GSVS/deflection-vertical.shtml the deflection of the vertical is described via "east" and "north" components. So I think, as suggested by @seisman, we should use "north-south" and update the remote dataset docs. For the PyGMT side, I am wondering, maybe component is better than direction for the parameter name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should at a note in the docs, that positive values mean a deflection to the east (north) and negative to the west (south) for the edefl (ndefl) dataset; similar as we did it for the "distance to shoreline" dataset.

east-west (edefl) north-south (ndefl)
edefl_grayc ndefl_grayc

Copy link
Member

Choose a reason for hiding this comment

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

For the PyGMT side, I am wondering, maybe component is better than direction for the parameter name.

Yes, I think so.

Maybe we should at a note in the docs, that positive values mean a deflection to the east (north) and negative to the west (south) for the edefl (ndefl) dataset; similar as we did it for the "distance to shoreline" dataset.

Sounds good.

pygmt/datasets/earth_deflection.py Outdated Show resolved Hide resolved
@yvonnefroehlich yvonnefroehlich changed the title WIP: Remote datasets: Add "IGPP Earth east-west and south-north deflection datases WIP: Remote datasets: Add "IGPP Earth east-west and north-south deflection datases Dec 28, 2024
@yvonnefroehlich yvonnefroehlich marked this pull request as ready for review December 28, 2024 15:07
@yvonnefroehlich yvonnefroehlich added the needs review This PR has higher priority and needs review. label Dec 29, 2024
@seisman seisman changed the title WIP: Remote datasets: Add "IGPP Earth east-west and north-south deflection datases Remote datasets: Add "IGPP Earth east-west and north-south deflection datases Dec 30, 2024
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Dec 30, 2024
@seisman seisman mentioned this pull request Dec 30, 2024
15 tasks
@seisman seisman changed the title Remote datasets: Add "IGPP Earth east-west and north-south deflection datases Remote datasets: Add "IGPP Earth east-west and north-south deflection" datasets Dec 30, 2024
@seisman seisman changed the title Remote datasets: Add "IGPP Earth east-west and north-south deflection" datasets Remote datasets: Add load_earth_deflection to load "IGPP Earth east-west and north-south deflection" datasets Dec 30, 2024
@seisman
Copy link
Member

seisman commented Dec 30, 2024

The CI failures on macOS are unrelated to changes in this PR and will be tracked in #3732.

Plan to merge this PR tomorrow if there are no further comments.

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 30, 2024
@seisman seisman merged commit 3134188 into main Dec 30, 2024
22 of 24 checks passed
@seisman seisman deleted the add-remote-earth-defl branch December 30, 2024 20:18
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.

3 participants