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 binstats #1652

Merged
merged 35 commits into from
Jun 13, 2022
Merged

Wrap binstats #1652

merged 35 commits into from
Jun 13, 2022

Conversation

willschlitzer
Copy link
Contributor

This wraps the GMT module gmtbinstats.

@willschlitzer willschlitzer added the feature Brand new feature label Dec 13, 2021
@willschlitzer willschlitzer added this to the 0.6.0 milestone Dec 13, 2021
@willschlitzer willschlitzer self-assigned this Dec 13, 2021
@willschlitzer willschlitzer mentioned this pull request Dec 13, 2021
5 tasks
@willschlitzer willschlitzer changed the title WIP: Wrap binstats Wrap binstats Dec 14, 2021
@willschlitzer willschlitzer marked this pull request as ready for review January 14, 2022 00:52
@seisman seisman modified the milestones: 0.6.0, 0.7.0 Mar 13, 2022
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Show resolved Hide resolved
pygmt/src/binstats.py Show resolved Hide resolved
pygmt/tests/test_binstats.py Outdated Show resolved Hide resolved
pygmt/tests/test_binstats.py Outdated Show resolved Hide resolved
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.

Looks good to me after fixing a typo.

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Jun 8, 2022
@seisman
Copy link
Member

seisman commented Jun 11, 2022

@GenericMappingTools/pygmt-maintainers Hopefully someone else can give this PR a final review.

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, @willschlitzer!

The only important comment here is to include @capitals.gmt in the download_test_data function since that's used for caching the files for the CI.

I agree with #1652 (comment) that the output to file option can be added later since it's more similar to #896 than #1536.

There's some work that needs to be done for managing required arguments, but I think we need some more consistency regarding that throughout the library and it's not a reason to hold this feature back.

pygmt/src/binstats.py Outdated Show resolved Hide resolved
pygmt/src/binstats.py Outdated Show resolved Hide resolved
Test binstats with no set outgrid.
"""
temp_grid = binstats(
data="@capitals.gmt",
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to pygmt.helpers.testing.download_test_data?

@willschlitzer willschlitzer merged commit 40ab2d9 into main Jun 13, 2022
@willschlitzer willschlitzer deleted the wrap/binstats branch June 13, 2022 07:52
@willschlitzer willschlitzer removed the final review call This PR requires final review and approval from a second reviewer label Jun 13, 2022
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
*Wrap gmtbinstats function
*Add test_binstats.py
*Add binstats to API index
*Add remote files "@capitals.gmt" to cached files

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Max Jones <[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.

None yet

4 participants