-
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
Initialize a GMTDataArrayAccessor #500
Conversation
An xarray accessor for GMT specific information! Currently holds the gridline/pixel registration and cartesian/geographic type properties. Created a new 'Metadata' section in the API docs for this, and moved info and grdinfo here.
Using a "setter" decorator to set/change class attributes. Wrote some unit tests to ensure code coverage if up to scratch, except that we can't test Pixel Registration properly yet.
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.
Ready for review. The current implementation actually works on both GMT 6.0 and 6.1, and I've avoided using the earth_relief
grid in the test. Once this is approved, I can continue on #476!
pygmt/modules.py
Outdated
# logging.warning( | ||
# msg="Cannot find a NetCDF source of xarray grid. " | ||
# f"Will fallback to using GMT's default setting: {default_reg_and_gtype}" | ||
# ) |
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.
Will uncomment this after we decide on #487.
Breaks backward compatibility with GMT 6.0, but simplifies the code (at some expense of readability). Co-Authored-By: Dongdong Tian <[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.
@seisman, ready to re-review. I was thinking of adding a __repr__
so that users can see what '0' or '1' means when they print the registration/gtype info, but that could go in a separate PR.
There's a lot we can add to this GMTDataArrayAccessor, and I've listed some in #499 (which I'll skip closing for now). This is really just the beginning 😄
pygmt/modules.py
Outdated
except KeyError: | ||
self._registration = 0 # Default to Gridline registration | ||
self._gtype = 0 # Default to Cartesian grid type | ||
# logging.warning( |
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.
Remove the warning?
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.
Ok, I've removed it for now, but will revisit it when we decide on #487. The intention is to not surprise users by enforcing a particular default registration/gtype (especially users used to using pixel registration/Cartesian coordinate data). The warning would ideally be surpressed by setting e.g. grid.gmt.registration = 1
, but I haven't quite worked out how to do so.
|
||
@registration.setter | ||
def registration(self, value): | ||
if value in (0, 1): |
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.
It may be more user friendly if we can also set the properties by grid.gmt.registration = "pixel"
and grid.gmt.gtype = "geographic"
, but I'm OK with the current boolean values, 0 and 1.
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.
Good idea. Let's leave it for now, but introduce this once we work out how to do the __repr__
as mentioned in #500 (review).
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.
Good to me.
Please change to a better title before merging. |
Oops sorry, missed your comment. |
Never mind. You can still change the draft release information directly. The title is not descriptive to me. |
Yep, will do that in the release notes. I think the draft one will be overwritten every time a PR is merged. |
Description of proposed changes
An xarray accessor for GMT specific information! Currently holds the gridline/pixel registration and cartesian/geographic type properties. Created a new 'Metadata' section in the API docs for this, and moved info and grdinfo here.
Enables functionality like this:
Will be extremely helpful for #476.
First step in fixing #499; Supersedes/Closes #494.
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.