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_dist" to load "GSHHG Earth distance to shoreline" dataset #3706

Merged
merged 16 commits into from
Dec 26, 2024

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Dec 22, 2024

Description of proposed changes

Add load_earth_dist to download the "GSHHG Earth distance to shoreline" dataset and load into a xarray.DataArray:

Adresses #2431

Preview: https://pygmt-dev--3706.org.readthedocs.build/en/3706/api/generated/pygmt.datasets.load_earth_dist.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.
  • Use underscores (not hyphens) in names of Python files and directories.

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 enhancement Improving an existing feature label Dec 22, 2024
@yvonnefroehlich
Copy link
Member Author

/format

@yvonnefroehlich yvonnefroehlich changed the title Add load_earth_dist to load GSHHG Earth Distance to Shoreline dataset in various resolutions Remote datasets: Add "load_earth_dist" to load "GSHHG Earth distance to shoreline" dataset Dec 22, 2024
@yvonnefroehlich yvonnefroehlich self-assigned this Dec 22, 2024
@seisman
Copy link
Member

seisman commented Dec 23, 2024

Thanks for working on this function. I feel we should first update the upstream documentation to clarify the following points:

  1. The distance unit is km
  2. The distances have both negative and positive values. Positive means land to coastline and negative means ocean to coastline.
  3. For the dataset image, maybe we should add the coastline to make the image more readable. Like this one:
    map

pygmt/datasets/earth_dist.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_dist.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_dist.py Outdated Show resolved Hide resolved
pygmt/helpers/caching.py Show resolved Hide resolved
assert data.attrs["units"] == "km"
assert data.attrs["horizontal_datum"] == "WGS84"
assert data.shape == (181, 361)
assert data.gmt.registration == 0
Copy link
Member

Choose a reason for hiding this comment

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

Need to update to the new syntax in #3696, depending on which PR is merged first.

pygmt/tests/test_datasets_earth_dist.py Outdated Show resolved Hide resolved
pygmt/tests/test_datasets_earth_dist.py Outdated Show resolved Hide resolved
@seisman seisman added feature Brand new feature and removed enhancement Improving an existing feature labels Dec 23, 2024
yvonnefroehlich and others added 3 commits December 23, 2024 22:49
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Dec 24, 2024
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. The dataset image will be updated automatically when the upstream PR is merged.

@seisman seisman mentioned this pull request Dec 24, 2024
49 tasks
@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Dec 25, 2024

Currently we use both, the abbreviation and the complete word for units in load_remote_datasets.py:

"earth_gebco": GMTRemoteDataset(
description="GEBCO Earth relief",
units="meters",

but

"earth_geoid": GMTRemoteDataset(
description="EGM2008 Earth geoid",
units="m",

I think we should decide on one way and update the dictionary in load_remote_datasets.py and the related tests.

Edit:
For the unit "meters", the complete word may be better to avoid a confusion with the "m" used in the resolution argument, which means arc-minutes.

@seisman
Copy link
Member

seisman commented Dec 25, 2024

That's a good point. The CF convention mentions that the unit string must be recognized by the UDUNIT package. I quickly read the UNUNIT documentation, I think valid values are meter, m and metre (without s). But the CF-convention has an example that uses meters (https://cfconventions.org/Data/cf-conventions/cf-conventions-1.12/cf-conventions.html#vertical-coordinate).

@seisman
Copy link
Member

seisman commented Dec 26, 2024

This section (http://cfconventions.org/Data/cf-conventions/cf-conventions-1.12/cf-conventions.html#_dimensional_vertical_coordinate) says:

Plural forms are also acceptable.

So I think meters/meter/m are fine. Let's use meters then.

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 26, 2024
@seisman seisman merged commit ebe9df1 into main Dec 26, 2024
24 checks passed
@seisman seisman deleted the add-remote-earth-dist branch December 26, 2024 02:51
@yvonnefroehlich
Copy link
Member Author

This section (http://cfconventions.org/Data/cf-conventions/cf-conventions-1.12/cf-conventions.html#_dimensional_vertical_coordinate) says:

Plural forms are also acceptable.

So I think meters/meter/m are fine. Let's use meters then.

Submitted PR #3725 for updating this for all datasets.

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