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

Wrap grdclip #1261

Merged
merged 36 commits into from
May 18, 2021
Merged

Wrap grdclip #1261

merged 36 commits into from
May 18, 2021

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented May 6, 2021

Wraps the gmt function grdclip.

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 6, 2021
@willschlitzer willschlitzer added this to the 0.4.0 milestone May 6, 2021
@willschlitzer willschlitzer self-assigned this May 6, 2021
@willschlitzer willschlitzer changed the title WIP: Wrap grdclip Wrap grdclip May 8, 2021
@willschlitzer willschlitzer marked this pull request as ready for review May 8, 2021 20:02
pygmt/src/grdclip.py Outdated Show resolved Hide resolved
pygmt/src/grdclip.py Outdated Show resolved Hide resolved
pygmt/src/grdclip.py Outdated Show resolved Hide resolved
pygmt/src/grdclip.py Outdated Show resolved Hide resolved
between : str or list or tuple
[*low*, *high*, *between*].
Set all data[i] >= *low* and <= *high* to *between*.
Repeat the option for as many intervals as are needed.
Copy link
Member

Choose a reason for hiding this comment

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

Repeat the option for as many intervals as are needed.

It's impossible to repeat the option in Python.

Copy link
Member

Choose a reason for hiding this comment

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

What if we could do it like frame (-B) in fig.basemap and pass in a list? Something like between=['0/2/1', '3/5/4']? Or a list of tuples like between=[(0, 2 ,1), (3, 5 ,4)] that expands to -Sb0/2/1 -Sb3/5/4 in GMT.

Copy link
Member

Choose a reason for hiding this comment

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

between=[(0, 2 ,1), (3, 5 ,4)] makes sense to me, but it needs more coding afforts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed in 1046419. Regarding @weiji14's suggestion, I'm assuming that would require a change to kwargs_to_strings()? Assuming so, I would prefer for this current method to be accepted as is, and a future pull request can be submitted once kwargs_to_strings is updated.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I was thinking of handling this feature by changing Sb='sequence'. Perhaps implementing a sequence_tuple? But yes, this can be done in a separate enhancement PR.

Copy link
Member

Choose a reason for hiding this comment

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

this can be done in a separate enhancement PR.

I agree.

pygmt/src/grdclip.py Outdated Show resolved Hide resolved
pygmt/src/grdclip.py Outdated Show resolved Hide resolved
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label May 17, 2021
Co-authored-by: Dongdong Tian <[email protected]>
Copy link
Member

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

Good work @willschlitzer! After you merge this in (remember to tidy the commit message), please open up an issue to handle the enhancement mentioned at #1261 (comment). Thanks!

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label May 18, 2021
@willschlitzer willschlitzer merged commit c7330da into master May 18, 2021
@willschlitzer willschlitzer deleted the wrap-grdclip branch May 18, 2021 06:55
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Wrap the gmt function grdclip and add tests for it.

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants