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

Set gridline (if available) as the default grid registration for remote datasets #2266

Merged
merged 44 commits into from
Dec 28, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
39203d5
update default registration for load_earth_relief
willschlitzer Dec 22, 2022
dd1b685
update default registration for load_earth_age
willschlitzer Dec 22, 2022
dcb09b5
Merge branch 'main' into load-remote-dataset/set-default-registration
seisman Dec 26, 2022
5d649e6
Update pygmt/datasets/load_remote_dataset.py
seisman Dec 26, 2022
ead76a0
Merge branch 'main' into load-remote-dataset/set-default-registration
willschlitzer Dec 27, 2022
24b8931
change if statement for default registration
willschlitzer Dec 27, 2022
bdfc550
add default registration testing for test_datasets_earth_age.py
willschlitzer Dec 27, 2022
7b0a219
update earth_age registration docstring
willschlitzer Dec 27, 2022
2fefe64
update earth_free_air_anomaly registration docstring
willschlitzer Dec 27, 2022
3442893
update test_datasets_earth_free_air_anomaly.py for default registration
willschlitzer Dec 27, 2022
d36f53d
update registration docstring in earth_geoid.py
willschlitzer Dec 27, 2022
24b3e73
add test to test_datasets_earth_geoid.py for default registration
willschlitzer Dec 27, 2022
2a64cf5
update registration docstring in earth_magnetic_anomaly.py
willschlitzer Dec 27, 2022
eaa57ae
add test to test_datasets_earth_magnetic_anomaly.py for default regis…
willschlitzer Dec 27, 2022
040883e
update registration docstring in earth_vertical_gravity_gradient.py
willschlitzer Dec 27, 2022
79ca10e
run make format
willschlitzer Dec 27, 2022
664d7da
add test for default registration to test_datasets_earth_vertical_gra…
willschlitzer Dec 27, 2022
c7fa8f5
update registration docstring to earth_relief.py
willschlitzer Dec 27, 2022
7ec384d
remove note in earth_relief.py
willschlitzer Dec 27, 2022
1fadc26
add tests for loading default registrations for 15s and 03s to test_d…
willschlitzer Dec 27, 2022
6f306fd
run make format
willschlitzer Dec 27, 2022
e24bcbc
Update pygmt/datasets/load_remote_dataset.py
willschlitzer Dec 27, 2022
087e6a9
remove test_earth_age_05m_with_region from test_datasets_earth_age.py
willschlitzer Dec 27, 2022
75519f7
remove test_earth_faa_05m_with_region from test_datasets_earth_free_a…
willschlitzer Dec 27, 2022
f668947
remove test_earth_geoid_05m_with_region from test_datasets_earth_geoi…
willschlitzer Dec 27, 2022
78fc8c1
remove test_earth_mag_05m_with_region from test_datasets_earth_magnet…
willschlitzer Dec 27, 2022
ae6176d
remove test_earth_vertical_gravity_gradient_05m_with_region from test…
willschlitzer Dec 27, 2022
3714ccc
remove test_earth_relief_05m_with_region from test_datasets_earth_rel…
willschlitzer Dec 27, 2022
9bfb317
uncomment pull_request for cache data
willschlitzer Dec 27, 2022
bea25d5
recomment pull_request for cache data
willschlitzer Dec 27, 2022
05f79a1
Apply suggestions from code review
willschlitzer Dec 28, 2022
8b09c33
Apply suggestions from code review
willschlitzer Dec 28, 2022
6a984e5
Merge branch 'main' into load-remote-dataset/set-default-registration
willschlitzer Dec 28, 2022
60c1eee
remove 05m mag4km test
willschlitzer Dec 28, 2022
224bbcf
change 05m to 01m resolution in test_datasets_earth_magnetic_anomaly.py
willschlitzer Dec 28, 2022
c624d43
change 05m to 01m resolution in test_datasets_earth_age.py
willschlitzer Dec 28, 2022
cad7796
change 05m to 01m resolution in test_datasets_earth_free_air_anomaly.py
willschlitzer Dec 28, 2022
706c5bc
change 05m to 01m resolution in test_datasets_earth_geoid.py
willschlitzer Dec 28, 2022
9cb34ec
change 05m to 01m resolution in test_datasets_earth_relief.py
willschlitzer Dec 28, 2022
6235bab
change 05m to 01m resolution in test_datasets_earth_vertical_gravity_…
willschlitzer Dec 28, 2022
f3fc043
Merge branch 'main' into load-remote-dataset/set-default-registration
willschlitzer Dec 28, 2022
f8553d5
Apply suggestions from code review
willschlitzer Dec 28, 2022
a2e508d
run make format
willschlitzer Dec 28, 2022
cedfa26
Always check if gridline is the default registration for 01d resolution
seisman Dec 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions pygmt/datasets/load_remote_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,20 +230,22 @@ def _load_remote_dataset(
The returned :class:`xarray.DataArray` doesn't support slice operation for
tiled grids.
"""

if registration in ("pixel", "gridline", None):
dataset = datasets[dataset_name]
if resolution not in dataset.resolutions.keys():
raise GMTInvalidInput(f"Invalid resolution '{resolution}'.")
if registration is None:
registration = dataset.resolutions[resolution].registrations[0]
Copy link
Member

Choose a reason for hiding this comment

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

I changed my mind and now I think your solution in #1929 (comment) is better, because it's more clear that "gridline" is the default registration.

In either case, I also think we should put gridline before pixel, because letter g is in front of letter p and gridline and pixel are internally represented as 0 and 1 in GMT.

Copy link
Contributor Author

@willschlitzer willschlitzer Dec 23, 2022

Choose a reason for hiding this comment

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

Okay. I will be back on December 26 and can update the PR then.

Copy link
Member

@seisman seisman Dec 26, 2022

Choose a reason for hiding this comment

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

In either case, I also think we should put gridline before pixel, because letter g is in front of letter p and gridline and pixel are internally represented as 0 and 1 in GMT.

I feel these changes should be done in a separate PR so that this PR is smaller and easier to review.

Edit: Done in #2276.

seisman marked this conversation as resolved.
Show resolved Hide resolved
if registration in ("pixel", "gridline"):
# If None, let GMT decide on Pixel/Gridline type
seisman marked this conversation as resolved.
Show resolved Hide resolved
reg = f"_{registration[0]}" if registration else ""
reg = f"_{registration[0]}"
else:
raise GMTInvalidInput(
f"Invalid grid registration: '{registration}', should be either "
"'pixel', 'gridline' or None. Default is None, where a "
"pixel-registered grid is returned unless only the "
"gridline-registered grid is available."
willschlitzer marked this conversation as resolved.
Show resolved Hide resolved
)
dataset = datasets[dataset_name]
if resolution not in dataset.resolutions.keys():
raise GMTInvalidInput(f"Invalid resolution '{resolution}'.")

if registration and (
registration not in dataset.resolutions[resolution].registrations
):
Expand Down
4 changes: 3 additions & 1 deletion pygmt/tests/test_datasets_earth_age.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ def test_earth_age_01d():
"""
Test some properties of the earth age 01d data.
"""
data = load_earth_age(resolution="01d", registration="gridline")
data = load_earth_age(resolution="01d")
assert data.name == "seafloor_age"
assert data.attrs["units"] == "Myr"
assert data.attrs["long_name"] == "age of seafloor crust"
assert data.attrs["horizontal_datum"] == "WGS84"
assert data.shape == (181, 361)
assert data.gmt.registration == 0
npt.assert_allclose(data.lat, np.arange(-90, 91, 1))
npt.assert_allclose(data.lon, np.arange(-180, 181, 1))
npt.assert_allclose(data.min(), 0.167381, rtol=1e-5)
Expand All @@ -51,6 +52,7 @@ def test_earth_age_01d_with_region():
resolution="01d", region=[-10, 10, -5, 5], registration="gridline"
)
assert data.shape == (11, 21)
assert data.gmt.registration == 0
npt.assert_allclose(data.lat, np.arange(-5, 6, 1))
npt.assert_allclose(data.lon, np.arange(-10, 11, 1))
npt.assert_allclose(data.min(), 11.293945)
Expand Down
6 changes: 3 additions & 3 deletions pygmt/tests/test_datasets_earth_relief.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,13 @@ def test_earth_relief_01d_igpp_synbath(data_source):
Test some properties of the earth relief 01d data with IGPP and SYNBATH
data.
"""
data = load_earth_relief(
resolution="01d", registration="gridline", data_source=data_source
)
data = load_earth_relief(resolution="01d", data_source=data_source)
assert data.name == "elevation"
assert data.attrs["units"] == "meters"
assert data.attrs["long_name"] == "Earth elevation relative to the geoid"
assert data.attrs["vertical_datum"] == "EGM96"
assert data.attrs["horizontal_datum"] == "WGS84"
assert data.gmt.registration == 0
assert data.shape == (181, 361)
npt.assert_allclose(data.lat, np.arange(-90, 91, 1))
npt.assert_allclose(data.lon, np.arange(-180, 181, 1))
Expand Down Expand Up @@ -132,6 +131,7 @@ def test_earth_gebcosi_15m_with_region():
data_source="gebcosi",
)
assert data.shape == (16, 8)
assert data.gmt.registration == 1
npt.assert_allclose(data.lat, np.arange(-87.875, -84, 0.25))
npt.assert_allclose(data.lon, np.arange(85.125, 87, 0.25))
npt.assert_allclose(data.min(), -531)
Expand Down