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.

Sign up for GitHub

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

Jump to bottom

Allow load_earth_relief() to load pixel or gridline registered data #509

Merged
merged 8 commits into from
Jul 10, 2020
Merged

Allow load_earth_relief() to load pixel or gridline registered data #509

merged 8 commits into from
Jul 10, 2020

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Jul 9, 2020 •
edited by seisman
Loading

Description of proposed changes

Somewhat backward compatible way of retrieving the earth_relief grids using GMT 6.0 or GMT 6.1. Tests on GMT 6.0 should still pass as usual in this PR, but there should be fewer failures on GMT 6.1 (except those due to the cpt change).

TODO:

  • Modify earth_relief.py to allow for getting pixel/gridline earth_relief grids (8017024) 53 test failures on GMT 6.1
  • Change unit tests to use load_earth_relief(registration="gridline") (9a74910) 39 test failures on GMT 6.1

Fixes (partially) #489

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 adding new functionality, add an example to docstrings or tutorials.

@weiji14
Allow getting pixel or gridline registered earth relief grids
8017024 
Somewhat backward compatible way of retrieving the `earth_relief` grids using GMT 6.0 or GMT 6.1.
@weiji14 weiji14 added the enhancement Improving an existing feature label Jul 9, 2020
@weiji14
Use load_earth_relief(pixel_reg=False) for grid in test
9a74910 
Refactored tests to explicitly use the gridline registered earth_relief grid, either by modifying the pytest fixture or adding a fixture. Also clarified that only gridline registered grids are available for GMT 6.0.
@weiji14 weiji14 mentioned this pull request Jul 9, 2020
Merged
5 tasks
@seisman
Copy link
Member

seisman commented Jul 9, 2020

@weiji14 Do you want to implement the registration option only, or do you also want to support region (by using grdcut in this PR?

@weiji14
Copy link
Member Author

weiji14 commented Jul 10, 2020

Let's keep this PR small. I actually have no idea how the grdcut and which commands go together, might be better for you to handle it.

@seisman
Copy link
Member

seisman commented Jul 10, 2020

OK. Please merge the master branch in then I can review it.

@weiji14
Copy link
Member Author

weiji14 commented Jul 10, 2020

Oops, should have used mkdir -p ~/.gmt instead of mkdir ~/.gmt at

pygmt/.github/workflows/ci_tests.yaml

Line 72 in a5af2dc

mkdir ~/.gmt ~/.gmt/cache ~/.gmt/server

Should I just fix it here?

seisman
seisman reviewed Jul 10, 2020
View reviewed changes
pygmt/datasets/earth_relief.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_relief.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_relief.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_relief.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_relief.py Outdated
"'pixel', 'gridline' or None. Default is None for pixel registration."
)

fname = which(f"@earth_relief_{resolution}{_registration}", download="u")
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use download="a" in GMT 6.1, but it's OK now.

pygmt/tests/test_datasets.py
@@ -80,9 +80,15 @@ def test_earth_relief_01d():

Copy link
Member

Choose a reason for hiding this comment

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

After bumping to GMT 6.1, we need to add a test with registration="pixel" or None.

@seisman

This comment has been minimized.

Sign in to view
@weiji14

This comment has been minimized.

Sign in to view
@seisman

This comment has been minimized.

Sign in to view
@seisman

This comment has been minimized.

Sign in to view
@seisman
Copy link
Member

seisman commented Jul 10, 2020

BTW, the PR title can be changed to "Allow load_earth_relief() to load pixel or gridline registered data". It's more readable to me.

@weiji14 weiji14 changed the title Allow getting pixel or gridline registered earth relief grids Allow load_earth_relief() to load pixel or gridline registered data Jul 10, 2020
@vercel vercel bot temporarily deployed to Preview July 10, 2020 04:17 Inactive
seisman
seisman approved these changes Jul 10, 2020
View reviewed changes
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.

I don't test it with GMT 6.1.0, but it looks good.

@weiji14 weiji14 merged commit ed82ae4 into master Jul 10, 2020
@weiji14 weiji14 deleted the earth_relief_p_or_g branch July 10, 2020 04:56
@seisman seisman added this to the 0.2.x milestone Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EJFielding EJFielding EJFielding left review comments

@seisman seisman seisman approved these changes

Assignees
No one assigned
Labels
enhancement Improving an existing feature
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@weiji14 @seisman @EJFielding