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

willschlitzer · 2022-12-22T13:48:03Z

This sets the default registration to "gridline" if both registration options are available.

Fixes #1929

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 commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

seisman · 2022-12-22T15:46:52Z

pygmt/datasets/load_remote_dataset.py

+    if registration is None:
+        registration = dataset.resolutions[resolution].registrations[0]


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.

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

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.

willschlitzer · 2022-12-25T02:09:32Z

Should I add tests using high-resolution grids to test that the functions default to the appropriate registrations when only one is available?

seisman · 2022-12-25T05:49:13Z

Should I add tests using high-resolution grids to test that the functions default to the appropriate registrations when only one is available?

Yes, please. The earth_relief_15s grid is good for testing.

pygmt/datasets/load_remote_dataset.py

…tration

…vity_gradient.py

…atasets_earth_relief.py

Co-authored-by: Dongdong Tian <[email protected]>

weiji14

Prefer explicit documentation on what resolutions default to gridline and which are pixel only. Feel free to change the wording if needed.

pygmt/datasets/earth_age.py

weiji14 · 2022-12-28T02:42:27Z

pygmt/datasets/earth_free_air_anomaly.py

+        ``"gridline"`` for gridline registration. Default is ``"gridline"``
+        when available.


What about rewording it explicitly to something like this?

Suggested change

``"gridline"`` for gridline registration. Default is ``"gridline"``

when available.

``"gridline"`` for gridline registration. Default is ``"gridline"``

for all resolutions except ``"01m"`` which is ``"pixel"`` only.

pygmt/datasets/earth_free_air_anomaly.py

pygmt/datasets/earth_geoid.py

pygmt/datasets/earth_magnetic_anomaly.py

pygmt/datasets/earth_relief.py

pygmt/datasets/earth_vertical_gravity_gradient.py

seisman · 2022-12-28T02:45:59Z

pygmt/tests/test_datasets_earth_age.py

-    assert data.sizes["lat"] == 121
-    assert data.sizes["lon"] == 121
-
-
 def test_earth_age_05m_without_region():


Please also update test_earth_age_05m_without_region to use the 01m resolution, so that we consistently use 01d and 01m in the tests.

Please also make the similar changes in other tests.

seisman · 2022-12-28T02:51:08Z

pygmt/tests/test_datasets_earth_magnetic_anomaly.py

@@ -146,3 +130,19 @@ def test_earth_mag4km_05m_with_region():
    assert data.lon.max() == -112


test_earth_mag4km_05m_with_region(): is still using the 05m grid.

Co-authored-by: Wei Ji <[email protected]>

…gradient.py

pygmt/tests/test_datasets_earth_magnetic_anomaly.py

Co-authored-by: Dongdong Tian <[email protected]>

willschlitzer · 2022-12-28T14:45:40Z

@seisman and @weiji14 Please feel free to commit any minor changes you suggest to this PR and #2241. I'm going to be away from my computer for a good portion of today, and don't want to hold up @yvonnefroehlich over something minor.

seisman · 2022-12-28T14:59:40Z

@seisman and @weiji14 Please feel free to commit any minor changes you suggest to this PR and #2241. I'm going to be away from my computer for a good portion of today, and don't want to hold up @yvonnefroehlich over something minor.

I've made the commit cedfa26 to make sure the 01d resolution always returns gridline-registered grids.

willschlitzer added 2 commits December 22, 2022 08:41

update default registration for load_earth_relief

39203d5

update default registration for load_earth_age

dd1b685

willschlitzer added this to the 0.8.0 milestone Dec 22, 2022

willschlitzer self-assigned this Dec 22, 2022

seisman reviewed Dec 22, 2022

View reviewed changes

yvonnefroehlich mentioned this pull request Dec 22, 2022

Release PyGMT v0.8.0 #2244

Closed

65 tasks

Merge branch 'main' into load-remote-dataset/set-default-registration

dcb09b5

seisman reviewed Dec 26, 2022

View reviewed changes

pygmt/datasets/load_remote_dataset.py Outdated Show resolved Hide resolved

Update pygmt/datasets/load_remote_dataset.py

5d649e6

seisman reviewed Dec 27, 2022

View reviewed changes

pygmt/datasets/load_remote_dataset.py Outdated Show resolved Hide resolved

pygmt/datasets/load_remote_dataset.py Outdated Show resolved Hide resolved

pygmt/datasets/load_remote_dataset.py Outdated Show resolved Hide resolved

willschlitzer and others added 18 commits December 27, 2022 06:26

Merge branch 'main' into load-remote-dataset/set-default-registration

ead76a0

change if statement for default registration

24b8931

add default registration testing for test_datasets_earth_age.py

bdfc550

update earth_age registration docstring

7b0a219

update earth_free_air_anomaly registration docstring

2fefe64

update test_datasets_earth_free_air_anomaly.py for default registration

3442893

update registration docstring in earth_geoid.py

d36f53d

add test to test_datasets_earth_geoid.py for default registration

24b3e73

update registration docstring in earth_magnetic_anomaly.py

2a64cf5

add test to test_datasets_earth_magnetic_anomaly.py for default regis…

eaa57ae

…tration

update registration docstring in earth_vertical_gravity_gradient.py

040883e

run make format

79ca10e

add test for default registration to test_datasets_earth_vertical_gra…

664d7da

…vity_gradient.py

update registration docstring to earth_relief.py

c7fa8f5

remove note in earth_relief.py

7ec384d

add tests for loading default registrations for 15s and 03s to test_d…

1fadc26

…atasets_earth_relief.py

run make format

6f306fd

Update pygmt/datasets/load_remote_dataset.py

e24bcbc

Co-authored-by: Dongdong Tian <[email protected]>

willschlitzer added 2 commits December 27, 2022 15:09

uncomment pull_request for cache data

9bfb317

recomment pull_request for cache data

bea25d5

weiji14 reviewed Dec 28, 2022

View reviewed changes

seisman reviewed Dec 28, 2022

View reviewed changes

willschlitzer and others added 11 commits December 28, 2022 09:18

Apply suggestions from code review

05f79a1

Co-authored-by: Wei Ji <[email protected]>

Apply suggestions from code review

8b09c33

Co-authored-by: Wei Ji <[email protected]>

Merge branch 'main' into load-remote-dataset/set-default-registration

6a984e5

remove 05m mag4km test

60c1eee

change 05m to 01m resolution in test_datasets_earth_magnetic_anomaly.py

224bbcf

change 05m to 01m resolution in test_datasets_earth_age.py

c624d43

change 05m to 01m resolution in test_datasets_earth_free_air_anomaly.py

cad7796

change 05m to 01m resolution in test_datasets_earth_geoid.py

706c5bc

change 05m to 01m resolution in test_datasets_earth_relief.py

9cb34ec

change 05m to 01m resolution in test_datasets_earth_vertical_gravity_…

6235bab

…gradient.py

Merge branch 'main' into load-remote-dataset/set-default-registration

f3fc043

seisman reviewed Dec 28, 2022

View reviewed changes

pygmt/tests/test_datasets_earth_magnetic_anomaly.py Outdated Show resolved Hide resolved

pygmt/tests/test_datasets_earth_magnetic_anomaly.py Outdated Show resolved Hide resolved

Apply suggestions from code review

f8553d5

Co-authored-by: Dongdong Tian <[email protected]>

willschlitzer and others added 2 commits December 28, 2022 09:46

run make format

a2e508d

Always check if gridline is the default registration for 01d resolution

cedfa26

seisman changed the title ~~Set default grid registration for remote datasets~~ Set gridline (if available) as the default grid registration for remote datasets Dec 28, 2022

seisman approved these changes Dec 28, 2022

View reviewed changes

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 28, 2022

weiji14 approved these changes Dec 28, 2022

View reviewed changes

seisman merged commit c833f88 into main Dec 28, 2022

seisman deleted the load-remote-dataset/set-default-registration branch December 28, 2022 15:17

seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 28, 2022

seisman mentioned this pull request Dec 28, 2022

load_earth_relief now can load gebco, gebcosi and synbath datasets with GMT 6.3 #2281

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

willschlitzer commented Dec 22, 2022

seisman Dec 22, 2022

willschlitzer Dec 23, 2022 •

edited

Loading

seisman Dec 26, 2022 •

edited

Loading

willschlitzer commented Dec 25, 2022 •

edited

Loading

seisman commented Dec 25, 2022 •

edited

Loading

weiji14 left a comment

weiji14 Dec 28, 2022

seisman Dec 28, 2022

seisman Dec 28, 2022

willschlitzer commented Dec 28, 2022

seisman commented Dec 28, 2022

		if registration is None:
		registration = dataset.resolutions[resolution].registrations[0]

		``"gridline"`` for gridline registration. Default is ``"gridline"``
		when available.

		@@ -146,3 +130,19 @@ def test_earth_mag4km_05m_with_region():
		assert data.lon.max() == -112

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

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

Conversation

willschlitzer commented Dec 22, 2022

seisman Dec 22, 2022

Choose a reason for hiding this comment

willschlitzer Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

seisman Dec 26, 2022 • edited Loading

Choose a reason for hiding this comment

willschlitzer commented Dec 25, 2022 • edited Loading

seisman commented Dec 25, 2022 • edited Loading

weiji14 left a comment

Choose a reason for hiding this comment

weiji14 Dec 28, 2022

Choose a reason for hiding this comment

seisman Dec 28, 2022

Choose a reason for hiding this comment

seisman Dec 28, 2022

Choose a reason for hiding this comment

willschlitzer commented Dec 28, 2022

seisman commented Dec 28, 2022

willschlitzer Dec 23, 2022 •

edited

Loading

seisman Dec 26, 2022 •

edited

Loading

willschlitzer commented Dec 25, 2022 •

edited

Loading

seisman commented Dec 25, 2022 •

edited

Loading