grdtrack: Fix the bug when profile is given by seisman · Pull Request #1867 · GenericMappingTools/pygmt

seisman · 2022-04-05T10:13:42Z

Description of proposed changes

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

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

pygmt/src/grdtrack.py

…e way

pygmt/src/grdtrack.py

weiji14 · 2022-05-10T13:14:42Z

pygmt/src/grdtrack.py

+    elif isinstance(grid, str):
+        try:
+            xr.open_dataarray(which(grid, download="a"))
+            is_a_grid = True


Does pygmt.which work with local NetCDF files? Also, not sure if I like this if-condition that requires opening a file using xr.open_dataarray to check if it is a grid. What about using grdinfo instead?

Does pygmt.which work with local NetCDF files?

Yes, it works.

Also, not sure if I like this if-condition that requires opening a file using xr.open_dataarray to check if it is a grid. What about using grdinfo instead?

grdinfo doesn't work. We don't catch the errors when reading a non-grid file:

pygmt.grdinfo("pygmt/tests/data/points.txt") grdinfo (gmtapi_import_grid): Not a supported grid format [pygmt/tests/data/points.txt] [Session pygmt-session (2)]: Error returned from GMT API: GMT_GRID_READ_ERROR (18) [Session pygmt-session (2)]: Error returned from GMT API: GMT_GRID_READ_ERROR (18)

Hmm, so pygmt.grdinfo produces a blank string for non-grid files. What about a check like:

if pygmt.grdinfo(grid) != "": is_a_grid = True else: is_a_grid = False

As an aside, we colud also think about making grdinfo produce proper Python errors as part of the grdinfo refactor (#593).

if pygmt.grdinfo(grid) != "": is_a_grid = True else: is_a_grid = False

It may work, but users will see the errors like:

grdinfo (gmtapi_import_grid): Not a supported grid format [pygmt/tests/data/points.txt] [Session pygmt-session (2)]: Error returned from GMT API: GMT_GRID_READ_ERROR (18) [Session pygmt-session (2)]: Error returned from GMT API: GMT_GRID_READ_ERROR (18)

Hmm yeah, I tried pygmt.grdinfo(grid="pygmt/tests/data/points.txt", verbose="q") which removed the first error line, but still getting two [Session pygmt-session (2)]: Error returned from GMT API: GMT_GRID_READ_ERROR (18) errors printed.

FYI, I've changed the line to

xr.open_dataarray(which(grid, download="a")).close()

to recycle any related resources.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

pygmt/src/grdtrack.py

weiji14 · 2022-05-27T19:06:30Z

pygmt/src/grdtrack.py

+        raise GMTInvalidInput("Must give 'points' or set 'profile'.")
+
+    if points is not None and kwargs.get("E") is not None:
+        raise GMTInvalidInput("Can't set both 'points' and 'profile'.")


Need a unit test to cover this line?

I added this unit test to cover this, but it's unclear to me why the coverage report says this line is not covered.

def test_grdtrack_set_points_and_profile(dataarray, dataframe): """ Run grdtrack but set both 'points' and 'profile'. """ with pytest.raises(GMTInvalidInput): grdtrack(grid=dataarray, points=dataframe, profile="BL/TR")

I tried running that line grdtrack(grid=dataarray, points=dataframe, profile="BL/TR") and got GMTInvalidInput: Please pass in a str to 'newcolname'. So maybe move these two lines a few lines above. See my suggested change at #1867 (comment)

pygmt/src/grdtrack.py

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

pygmt/src/grdtrack.py

weiji14 · 2022-06-04T17:56:14Z

pygmt/src/grdtrack.py

+        raise GMTInvalidInput("Must give 'points' or set 'profile'.")
+
+    if points is not None and kwargs.get("E") is not None:
+        raise GMTInvalidInput("Can't set both 'points' and 'profile'.")


I tried running that line grdtrack(grid=dataarray, points=dataframe, profile="BL/TR") and got GMTInvalidInput: Please pass in a str to 'newcolname'. So maybe move these two lines a few lines above. See my suggested change at #1867 (comment)

pygmt/src/grdtrack.py

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

weiji14

Should be ok if all tests pass.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

grdtrack: Fix the bug when profile is given

0c79da4

weiji14 reviewed Apr 5, 2022

View reviewed changes

pygmt/src/grdtrack.py Show resolved Hide resolved

weiji14 mentioned this pull request Apr 5, 2022

Release PyGMT v0.6.1 #1856

Closed

30 tasks

weiji14 added the bug Something isn't working label Apr 5, 2022

seisman mentioned this pull request Apr 6, 2022

grdtrack not working with "profile" option #1827

Closed

seisman added this to the 0.7.0 milestone Apr 6, 2022

seisman added 2 commits April 9, 2022 22:41

Merge branch 'main' into fix-grdtrack-profile

0ade6a3

Merge branch 'main' into fix-grdtrack-profile

a36482c

vercel bot temporarily deployed to Preview April 12, 2022 12:31 Inactive

Merge branch 'main' into fix-grdtrack-profile

66d2492

vercel bot temporarily deployed to Preview May 4, 2022 08:24 Inactive

seisman added 2 commits May 4, 2022 16:27

Update grdtrack definition so that points is not required

9da93fc

Add two tests for invalid inputs

78d98ad

vercel bot temporarily deployed to Preview May 4, 2022 08:37 Inactive

seisman added 2 commits May 7, 2022 16:46

Merge branch 'main' into fix-grdtrack-profile

0aa1165

Add a test for passing profile to grdtrack

1080ace

vercel bot temporarily deployed to Preview May 7, 2022 08:58 Inactive

vercel bot temporarily deployed to Preview May 7, 2022 10:45 Inactive

seisman marked this pull request as ready for review May 7, 2022 11:50

Change the function definition to 'grid, points=None' in an compatibl…

55662f6

…e way

seisman force-pushed the fix-grdtrack-profile branch from d70444f to 55662f6 Compare May 7, 2022 11:51

vercel bot temporarily deployed to Preview May 7, 2022 12:01 Inactive

Fix a linting issue

1a068ba

vercel bot temporarily deployed to Preview May 8, 2022 10:28 Inactive

weiji14 reviewed May 10, 2022

View reviewed changes

Update pygmt/src/grdtrack.py

a9038d3

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

vercel bot temporarily deployed to Preview May 10, 2022 14:29 Inactive

seisman commented May 11, 2022

View reviewed changes

pygmt/src/grdtrack.py Outdated Show resolved Hide resolved

seisman commented May 11, 2022

View reviewed changes

pygmt/src/grdtrack.py Outdated Show resolved Hide resolved

Merge branch 'main' into fix-grdtrack-profile

a0ec99a

vercel bot temporarily deployed to Preview May 13, 2022 04:25 Inactive

Apply suggestions from code review

2770a6c

vercel bot temporarily deployed to Preview May 17, 2022 01:51 Inactive

Merge branch 'main' into fix-grdtrack-profile

eb2cac7

vercel bot temporarily deployed to Preview May 22, 2022 06:50 Inactive

Merge branch 'main' into fix-grdtrack-profile

8aaebef

seisman requested a review from weiji14 May 27, 2022 05:21

vercel bot temporarily deployed to Preview May 27, 2022 05:30 Inactive

weiji14 reviewed May 27, 2022

View reviewed changes

Apply suggestions from code review

16dc554

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

vercel bot temporarily deployed to Preview May 28, 2022 03:08 Inactive

seisman commented May 29, 2022

View reviewed changes

pygmt/src/grdtrack.py Outdated Show resolved Hide resolved

seisman commented May 29, 2022

View reviewed changes

pygmt/src/grdtrack.py Outdated Show resolved Hide resolved

Catch both ValueError and OSError

f312d64

vercel bot temporarily deployed to Preview May 29, 2022 14:31 Inactive

seisman commented May 29, 2022

View reviewed changes

pygmt/src/grdtrack.py Outdated Show resolved Hide resolved

Update pygmt/src/grdtrack.py

27aec05

vercel bot temporarily deployed to Preview May 29, 2022 14:42 Inactive

weiji14 reviewed Jun 4, 2022

View reviewed changes

Update pygmt/src/grdtrack.py

012fa2c

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

weiji14 approved these changes Jun 5, 2022

View reviewed changes

weiji14 added the final review call This PR requires final review and approval from a second reviewer label Jun 5, 2022

vercel bot temporarily deployed to Preview June 5, 2022 03:27 Inactive

Merge branch 'main' into fix-grdtrack-profile

4c809ce

vercel bot temporarily deployed to Preview June 5, 2022 08:31 Inactive

seisman merged commit ce73717 into main Jun 6, 2022

seisman deleted the fix-grdtrack-profile branch June 6, 2022 05:48

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

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022

grdtrack: Fix the bug when profile is given (GenericMappingTools#1867)

17ac269

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

seisman mentioned this pull request Jan 7, 2023

pygmt.grdtrack: Remove the warning about the incorrect parameter order of 'points, grid' (warned since v0.7.0) #2312

Merged

7 tasks

Conversation

seisman commented Apr 5, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weiji14 Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weiji14 Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

weiji14 Jun 4, 2022 •

edited

Loading

weiji14 Jun 4, 2022 •

edited

Loading