-
Notifications
You must be signed in to change notification settings - Fork 224
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
grdtrack: Fix the bug when profile is given #1867
Conversation
d70444f
to
55662f6
Compare
pygmt/src/grdtrack.py
Outdated
elif isinstance(grid, str): | ||
try: | ||
xr.open_dataarray(which(grid, download="a")) | ||
is_a_grid = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usinggrdinfo
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
pygmt/src/grdtrack.py
Outdated
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'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a unit test to cover this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Co-authored-by: Wei Ji <[email protected]>
pygmt/src/grdtrack.py
Outdated
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'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Co-authored-by: Wei Ji <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ok if all tests pass.
Co-authored-by: Wei Ji <[email protected]>
Description of proposed changes
Fixes #1827.
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version