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 blockmode #1456

Merged
merged 16 commits into from
Sep 15, 2021
Merged

Wrap blockmode #1456

merged 16 commits into from
Sep 15, 2021

Conversation

arleaman
Copy link
Contributor

@arleaman arleaman commented Aug 19, 2021

Description of proposed changes

Wrap blockmode

Addresses #1091

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

@maxrjones
Copy link
Member

/format

@weiji14 weiji14 added the feature Brand new feature label Aug 20, 2021
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, @arleaman! I made a couple small formatting suggestions. Apart from that, there are two more steps needed before adding this module to PyGMT:

  1. Add blockmode to /doc/api/index.rst in line 84.
  2. Add a test for blockmode. I can provide some instructions for how to add a test for this module, if you expect to have time to work with us on finishing up the pull request?

pygmt/src/blockm.py Outdated Show resolved Hide resolved
pygmt/src/blockm.py Outdated Show resolved Hide resolved
@seisman seisman added this to the 0.5.0 milestone Aug 29, 2021
@seisman
Copy link
Member

seisman commented Aug 29, 2021

Ping @arleaman to finalize this PR if available.

arleaman and others added 4 commits September 1, 2021 09:48
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
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 except two minor suggestions.

pygmt/src/blockm.py Outdated Show resolved Hide resolved
pygmt/src/blockm.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 Sep 1, 2021
arleaman and others added 2 commits September 1, 2021 16:09
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
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.

🎉 Thanks @arleaman, we'll merge this in about 24 hours if there are no other last minute changes required.

@arleaman
Copy link
Contributor Author

arleaman commented Sep 1, 2021

Thanks @meghanrjones! I'll be happy to add tests for the module with some additional guidance!

  1. Add a test for blockmode. I can provide some instructions for how to add a test for this module, if you expect to have time to work with us on finishing up the pull request?

@maxrjones
Copy link
Member

maxrjones commented Sep 1, 2021

Thanks, @arleaman! Before adding tests, let's check with the other reviewers whether a test is needed for this new function or the tests for blockmedian and blockmean provide sufficient coverage. What do you think, @weiji14 and @seisman?

@seisman
Copy link
Member

seisman commented Sep 2, 2021

Thanks, @arleaman! Before adding tests, let's check with the other reviewers whether a test is needed for this new function or the tests for blockmedian and blockmean provide sufficient coverage. What do you think, @weiji14 and @seisman?

I think we still need one test for blockmode to make sure that the function can be called.

@maxrjones
Copy link
Member

Great, let's add a test! @arleaman, here is some guidance:

  1. Since the modules blockmean and blockmode are so similar, we can add the test to the file with the blockmean tests rather than creating a new file with separate imports/datasets for blockmode. To do this, we need to first generalize the blockmean tests filename (currently pygmt/tests/test_blockmean.py):
    cd <path-to-pygmt-repository>
    git checkout wrap-blockmode
    git mv pygmt/tests/test_blockmean.py pygmt/tests/test_blockm.py # Renames the file
    git status # Check that the change is ready to be committed
    git commit -m "Rename test_blockmean.py to test_blockm.py"
    
  2. Generalize the docstring in pygmt/tests/test_blockm.py by updating L2 "Tests for blockmean." to "Tests for blockmean and blockmode."
  3. Add blockmode to the from pygmt import blockmean statement on L9
  4. Create a new test for blockmode. You can use the same structure one of the tests for blockmean:
    • Copy/paste the first test in pygmt/tests/test_blockm.py (test_blockmean_input_dataframe) and change the name for the new function to test_blockmode_input_dataframe
    • Change the function call from blockmean(table=dataframe...) to blockmode(table=dataframe...)
    • Update the last line in the test (npt.assert_allclose(...)) so that the output is correct for blockmode rather than blockmean (i.e., change -384.0 to -385.0)

@maxrjones maxrjones removed the final review call This PR requires final review and approval from a second reviewer label Sep 2, 2021
@arleaman
Copy link
Contributor Author

Great, let's add a test! @arleaman, here is some guidance:

Thanks for the help @meghanrjones!

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.

No problem and thanks for making the changes! 😄

I made a couple suggestions related to adding some new common options.

Also, could you please merge the latest changes into your branch by clicking update branch?

image

pygmt/src/blockm.py Show resolved Hide resolved
pygmt/src/blockm.py Show resolved Hide resolved
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
arleaman and others added 3 commits September 13, 2021 14:54
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
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! 🎉

I'll leave this open for a short while in case others have any last minute comments.

@maxrjones maxrjones added the final review call This PR requires final review and approval from a second reviewer label Sep 13, 2021
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 14, 2021
@seisman seisman merged commit a99533f into GenericMappingTools:main Sep 15, 2021
@maxrjones maxrjones added the eswn-workshop Good issues for first-contributions during the ESWN-PyGMT workshop label Sep 27, 2021
seisman added a commit that referenced this pull request Oct 3, 2021
The alias of d is nodata in other modules, but was incorrectly set to data in blockmean and blockmode. This PR fixes the problem.

blockmode was wrapped in #1456 (after v0.4.1), blockmean was wrapped in #1092 but the wrong alias was added in #1500 (after v0.4.1). Thus, the change won't go into the deprecation cycle.
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…ngTools#1563)

The alias of d is nodata in other modules, but was incorrectly set to data in blockmean and blockmode. This PR fixes the problem.

blockmode was wrapped in GenericMappingTools#1456 (after v0.4.1), blockmean was wrapped in GenericMappingTools#1092 but the wrong alias was added in GenericMappingTools#1500 (after v0.4.1). Thus, the change won't go into the deprecation cycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eswn-workshop Good issues for first-contributions during the ESWN-PyGMT workshop feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants