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 nearneighbor #1379

Merged

Conversation

JamieJQuinn
Copy link
Contributor

@JamieJQuinn JamieJQuinn commented Jul 8, 2021

Description of proposed changes

This PR adds an implementation of nearneighbor, mirroring that of the already implemented surface.

The documentation has been cobbled together from similar portions in
surface and from the official GMT documentation on
nearneighbor
.
Additional long-form flags have been added, reflecting the flags
particularly useful for nearneighbor, mainly -Ssearch_radius,
-Eempty and -Nsectors.

Tests have been blatantly copied from those testing surface, since the
two functions should operate broadly similarly.

The nearneighbor command is particularly important for remove-restore, an algorithm used to combine different datasets into one grid. In my case, this is implemented in a package I manage, pyCascadia, which is using pyGMT to implement remove-restore for the purposes of combining bathymetry datasets. It would be useful to pyCascadia to have nearneighbor implemented to the same degree as e.g. surface.

Preview docs at https://pygmt-git-fork-jamiejquinn-feature-implement-nearneighbour-gmt.vercel.app/api/generated/pygmt.nearneighbor.html

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

The implementation mirrors that of the already implemented surface.

The documentation has been cobbled together from similar portions in
surface and from the [official GMT documentation on
nearneighbor](https://docs.generic-mapping-tools.org/latest/nearneighbor.html).
Additional long-form flags have been added, reflecting the flags
particularly useful for nearneighbor, mainly `-Ssearch_radius`,
`-Eempty` and `-Nsectors`.

Tests have been blatently copied from those testing surface, since the
two functions should operate broadly similarly.
@welcome
Copy link

welcome bot commented Jul 8, 2021

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

  • If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
  • All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
  • No matter what, we are really grateful that you put in the effort to do this! 🎉

@weiji14 weiji14 added the feature Brand new feature label Jul 8, 2021
@weiji14 weiji14 added this to the 0.5.0 milestone Jul 8, 2021
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.

Hi @JamieJQuinn, thank you for opening up this PR! I see that you have a parallel implementation over at UCL/pyCascadia#65, so good on you for bringing this into PyGMT. Overall, your implementation is actually solid and works properly. I have listed a few suggested changes below, mostly related to modernization of the code style (the surface module you based your code on hasn't really been updated since 2019).

If you agree to the changes, feel free to do a "Commit suggestion" directly, or pool several together using "Add suggestion to batch" before committing. See also https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request#applying-a-suggested-change.

image

pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
There are many pieces of older code mirroring those found in the implementation of `surface`. These have since been updating in other parts of pyGMT. Most of the review suggestions modernise this PR.

Co-authored-by: Wei Ji <[email protected]>
@JamieJQuinn
Copy link
Contributor Author

@weiji14 cheers for the suggestions!

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.

I'm more or less happy with the implementation here, aside from a couple of tiny things. Would appreciate it if someone from @GenericMappingTools/pygmt-maintainers takes a look too.

pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@willschlitzer willschlitzer 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 (but agree with @weiji14's two suggestions)!

Enhance nearneighbour documentation

Co-authored-by: Wei Ji <[email protected]>
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Outdated Show resolved Hide resolved
pygmt/tests/test_nearneighbor.py Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Sep 20, 2021

Maintainers, please give this a final review (docs preview at https://pygmt-git-fork-jamiejquinn-feature-implement-nearneighbour-gmt.vercel.app/api/generated/pygmt.nearneighbor.html), and if there are no major objections, will merge in ~24 hours 🚀

@weiji14 weiji14 requested a review from a team September 20, 2021 23:10
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 for two suggestions on docstrings.

pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
pygmt/src/nearneighbor.py Outdated Show resolved Hide resolved
@weiji14 weiji14 merged commit 8eb055d into GenericMappingTools:main Sep 23, 2021
@welcome
Copy link

welcome bot commented Sep 23, 2021

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Sep 23, 2021
@weiji14
Copy link
Member

weiji14 commented Sep 23, 2021

Thanks again for your contribution @JamieJQuinn 😄 The nearneighbor function will be included in PyGMT v0.5.0, but if you can't wait for that, feel free to install the development version following https://www.pygmt.org/dev/install.html#using-pip:

pip install --pre --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple pygmt

This will install the version of PyGMT from TestPyPI at https://test.pypi.org/project/pygmt/0.4.2.dev64. Oh, and you're welcome to send in new feature requests or ideas to our issue tracker at https://github.com/GenericMappingTools/pygmt/issues. Looking forward to seeing you around more!

@willschlitzer
Copy link
Contributor

@JamieJQuinn Feel free to open up a pull request to add yourself to AUTHORS.md if you would like! We have our v0.5.0 release scheduled for tomorrow that will include nearneighbor.

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Wrapping the nearneighbor function which grids table data using a
"Nearest neighbor" algorithm. Official GMT documentation is at 
https://docs.generic-mapping-tools.org/6.2/nearneighbor.html.
Aliased empty (E), spacing (I), sectors (N), search_radius (S).

* Add nearneighbor to API index
* Add similar figure to that found in GMTs nearneighbor documentation
* Ignore flake8 error using noqa W505
* Shorten line length to under 100 using the dev image
* Merge two tests using pytest parametrize

Check that numpy.array and xarray.Dataset inputs work.

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Meghan Jones <[email protected]>
Co-authored-by: Michael Grund <[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
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants