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.

Sign up for GitHub

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

Jump to bottom

Wrap grd2xyz #1284

Merged
merged 66 commits into from
Sep 22, 2021
Merged

Wrap grd2xyz #1284

merged 66 commits into from
Sep 22, 2021

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented May 22, 2021 •
edited by weiji14
Loading

This pull request wraps the grd2xyz module.

Fixes #

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

@willschlitzer willschlitzer added the feature Brand new feature label May 22, 2021
@willschlitzer willschlitzer added this to the 0.4.0 milestone May 22, 2021
@willschlitzer willschlitzer self-assigned this May 22, 2021
seisman
seisman reviewed May 24, 2021
View reviewed changes
pygmt/tests/test_grd2xyz.py Outdated
Make sure grd2xyz works as expected.
"""
xyz_data = grd2xyz(grid=grid)
assert xyz_data.strip().split("\n") == [
Copy link
Member

Choose a reason for hiding this comment

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

Quick comment: I don't think a string-type output is useful. The function should either return numpy 1d/2d arrays or pandas.DataFrame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seisman could you look at what I just added? As far as I can tell it is working correctly.

Copy link
Member

@weiji14 weiji14 May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We might need to open an issue to discuss the output format. Currently, the blockm*/grdtrack modules simply outputs pandas.DataFrame/filenames (see #1099) automatically depending on the input type (2 options). The implementation @willschlitzer made at bf58dc2 and 68fab7c returns either numpy/pandas/str depending on the user input (3 options). Best to be consistent across PyGMT on the types of outputs users can expect.

@weiji14
Copy link
Member

weiji14 commented May 26, 2021

@willschlitzer, I'll set this PR to 'draft' mode to conserve CI resources since it might take a while to settle on the output format types (as mentioned in #1284 (comment)).

Also on a related note, please go easy on clicking the 'Update branch' button since it uses CI resources and also triggers notifications to all maintainers (double notifications if the tests happen to fail). I know you're keen on getting more PRs in (and we do welcome new features!), but there are better ways to merge in a sequence of PRs one after another.

For example with 5 PRs, instead of clicking 'Update branch' 5+4+3+2+1=15 times (updating every PR after one is merged), we can click 'Update branch' 1+1+1+1+1=5 times (updating only the 1 PR that is going to be merged next).

@weiji14 weiji14 marked this pull request as draft May 26, 2021 10:31
@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Sep 20, 2021
@weiji14 weiji14 mentioned this pull request Sep 20, 2021
Merged
5 tasks
seisman
seisman reviewed Sep 21, 2021
View reviewed changes
pygmt/src/grd2xyz.py Outdated Show resolved Hide resolved
pygmt/src/grd2xyz.py Outdated Show resolved Hide resolved
@willschlitzer @weiji14 @seisman
Apply suggestions from code review
e3336c4 
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
seisman
seisman reviewed Sep 22, 2021
View reviewed changes
pygmt/src/grd2xyz.py Outdated Show resolved Hide resolved
pygmt/src/grd2xyz.py Outdated Show resolved Hide resolved
pygmt/src/grd2xyz.py Outdated Show resolved Hide resolved
pygmt/tests/test_grd2xyz.py Outdated Show resolved Hide resolved
pygmt/tests/test_grd2xyz.py Outdated Show resolved Hide resolved
seisman
seisman approved these changes Sep 22, 2021
View reviewed changes
pygmt/src/grd2xyz.py Outdated Show resolved Hide resolved
pygmt/tests/test_grd2xyz.py Outdated Show resolved Hide resolved
@willschlitzer @seisman
Update pygmt/src/grd2xyz.py
6731814 
Co-authored-by: Dongdong Tian <[email protected]>
@seisman
Copy link
Member

seisman commented Sep 22, 2021 •
edited
Loading

I think this PR is already good for merging, but it would be better if @meghanrjones can also give it a final review, since @meghanrjones made a lot of suggestions in this PR.

maxrjones
maxrjones approved these changes Sep 22, 2021
View reviewed changes
Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Great work, Will!

@seisman seisman merged commit cbe44aa into main Sep 22, 2021
@seisman seisman deleted the wrap-grd2xyz branch September 22, 2021 23:22
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 22, 2021
@weiji14 weiji14 mentioned this pull request Sep 23, 2021
Open
4 tasks
@willschlitzer willschlitzer mentioned this pull request Sep 23, 2021
Merged
5 tasks
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
@willschlitzer @maxrjones
@weiji14 @seisman
Wrap grd2xyz (GenericMappingTools#1284)
764ca7a 
Co-authored-by: Meghan Jones <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiji14 weiji14 weiji14 approved these changes

@maxrjones maxrjones maxrjones approved these changes

@seisman seisman seisman approved these changes

Assignees

@willschlitzer willschlitzer

Labels
feature Brand new feature
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@willschlitzer @weiji14 @maxrjones @seisman