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

Allow GMTDataArrayAccessor to work on sliced datacubes #1581

Merged
merged 5 commits into from
Oct 28, 2021

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Oct 13, 2021

Description of proposed changes

If grdinfo can't read the source NetCDF file as it is an n-dimensional datacube (instead of a 2D grid), we fallback to setting the default gridline registration and Cartesian grid type.

Note that a warning like grdinfo [WARNING]: Detected a data cube (eraint_uvz.nc) but -Q not set - skipping will still be emitted, but running grid.gmt.registration should still work. Perhaps a better way would be to try to use grdinfo -Q, but then we would somehow need to know which variable within the datacube people have sliced.

Fixes #1578, Addresses #524 (comment)

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.

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

@weiji14 weiji14 added the bug Something isn't working label Oct 13, 2021
@weiji14 weiji14 added this to the 0.5.0 milestone Oct 13, 2021
@weiji14 weiji14 self-assigned this Oct 13, 2021
If `grdinfo` can't read the source NetCDF file as it
is an n-dimensional datacube (instead of a 2D grid),
we fallback to setting the default gridline registration
and Cartesian grid type.
@weiji14 weiji14 marked this pull request as ready for review October 22, 2021 00:14
So that the file can be deleted on Windows.
@@ -34,7 +34,7 @@ def __init__(self, xarray_obj):
self._registration, self._gtype = map(
int, grdinfo(self._source, per_column="n", o="10,11").split()
)
except KeyError:
except (KeyError, ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand it, previous imports of sliced datacubes would result in a ValueError, so this makes an exception for it and sets the grid registration and type to a default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Sliced datacubes will fallback to using the default grid registration and type instead of raising a ValueError.

Copy link
Contributor

@willschlitzer willschlitzer 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 know when a sliced datacube would be used or what type of error it would cause (I'm assuming a ValueError based upon the changes to accessors.py), but on the grounds that that this adds a passing test, so I assume it works as intended.

@willschlitzer willschlitzer added the final review call This PR requires final review and approval from a second reviewer label Oct 27, 2021
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.

The PR looks good to me, but do we want to cache the data file?

Comment on lines +82 to +85
fname = which(
"https://github.com/pydata/xarray-data/raw/master/eraint_uvz.nc",
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.

Do we want to cache this file in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably don't need to, since this file is on GitHub, and our CI is on GitHub too, so if GitHub is down, nothing will work 😆

@weiji14 weiji14 mentioned this pull request Oct 28, 2021
35 tasks
@weiji14 weiji14 merged commit 70f6f09 into main Oct 28, 2021
@weiji14 weiji14 deleted the accessor/sliced_datacube_bugfix branch October 28, 2021 21:54
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 18, 2022
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…gTools#1581)

If `grdinfo` can't read the source NetCDF file as it
is an n-dimensional datacube (instead of a 2D grid),
we fallback to setting the default gridline registration
and Cartesian grid type.

* Use with statement to ensure the grid is properly closed

So that the file can be deleted on Windows.

Co-authored-by: Will Schlitzer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xarray DataArray attributes breaking grdimage
4 participants