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

Initialize a GMTDataArrayAccessor #500

Merged
merged 9 commits into from
Jul 11, 2020
Merged

Initialize a GMTDataArrayAccessor #500

merged 9 commits into from
Jul 11, 2020

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Jun 29, 2020

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:

print(grid.gmt.registration)
1  # for pixel registration
print(grid.gmt.gtype)
0  # for cartesian coordinates

Will be extremely helpful for #476.

First step in fixing #499; Supersedes/Closes #494.

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.

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.
@weiji14 weiji14 added the feature Brand new feature label Jun 29, 2020
@weiji14 weiji14 marked this pull request as draft June 29, 2020 11:36
@weiji14 weiji14 changed the title Initialize a GMTDataArrayAccessor WIP: Initialize a GMTDataArrayAccessor Jun 29, 2020
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.
@weiji14 weiji14 mentioned this pull request Jul 2, 2020
10 tasks
@weiji14 weiji14 changed the title WIP: Initialize a GMTDataArrayAccessor Initialize a GMTDataArrayAccessor Jul 9, 2020
@weiji14 weiji14 marked this pull request as ready for review July 9, 2020 10:38
Copy link
Member Author

@weiji14 weiji14 left a 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
Comment on lines 248 to 251
# logging.warning(
# msg="Cannot find a NetCDF source of xarray grid. "
# f"Will fallback to using GMT's default setting: {default_reg_and_gtype}"
# )
Copy link
Member Author

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.

pygmt/modules.py Outdated Show resolved Hide resolved
pygmt/modules.py Outdated Show resolved Hide resolved
Breaks backward compatibility with GMT 6.0, but simplifies the code (at 
some expense of readability).

Co-Authored-By: Dongdong Tian <[email protected]>
Copy link
Member Author

@weiji14 weiji14 left a 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 Show resolved Hide resolved
pygmt/modules.py Show resolved Hide resolved
pygmt/modules.py Outdated
except KeyError:
self._registration = 0 # Default to Gridline registration
self._gtype = 0 # Default to Cartesian grid type
# logging.warning(
Copy link
Member

Choose a reason for hiding this comment

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

Remove the warning?

Copy link
Member Author

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):
Copy link
Member

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.

Copy link
Member Author

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).

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.

Good to me.

@seisman seisman added this to In progress in Release v0.2.x via automation Jul 11, 2020
@seisman seisman added this to the 0.2.x milestone Jul 11, 2020
@seisman
Copy link
Member

seisman commented Jul 11, 2020

Please change to a better title before merging.

@weiji14 weiji14 merged commit b3aa16d into master Jul 11, 2020
Release v0.2.x automation moved this from In progress to Done Jul 11, 2020
@weiji14 weiji14 deleted the gmt-xr-accessor branch July 11, 2020 23:19
@weiji14
Copy link
Member Author

weiji14 commented Jul 11, 2020

Oops sorry, missed your comment.

@seisman
Copy link
Member

seisman commented Jul 11, 2020

Never mind. You can still change the draft release information directly. The title is not descriptive to me.

@weiji14
Copy link
Member Author

weiji14 commented Jul 11, 2020

Yep, will do that in the release notes. I think the draft one will be overwritten every time a PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants