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

Expand table-like input options for sphdistance #1491

Merged
merged 22 commits into from
Sep 30, 2021
Merged

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented Sep 11, 2021

This pull request adds data to the parameter list in the docstring and moves the sphdistance function to the tabular data section in the index.

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 enhancement Improving an existing feature label Sep 11, 2021
@willschlitzer willschlitzer added this to the 0.5.0 milestone Sep 11, 2021
@willschlitzer willschlitzer self-assigned this Sep 11, 2021
pygmt/src/sphdistance.py Outdated Show resolved Hide resolved
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.

Could you also:

  • Rename table to data at L25, L61, and also in pygmt/tests/test_sphdistance.py
  • At L52: Insert blank line between:
       Return type depends on whether the ``outgrid`` parameter is set:
        - :class:`xarray.DataArray` if ``outgrid`` is not set

to prevent a Sphinx warning pygmt/pygmt/src/sphdistance.py:docstring of pygmt.src.sphdistance.sphdistance:67: WARNING: Unexpected indentation

pygmt/src/sphdistance.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added the skip-changelog Skip adding Pull Request to changelog label Sep 17, 2021
@seisman
Copy link
Member

seisman commented Sep 23, 2021

Do you want to add "xyz" input support (like #1531) in this PR or a separate PR?

@willschlitzer
Copy link
Contributor Author

Do you want to add "xyz" input support (like #1531) in this PR or a separate PR?

I can give it a try.

pygmt/src/sphdistance.py Outdated Show resolved Hide resolved
pygmt/src/sphdistance.py Outdated Show resolved Hide resolved
pygmt/src/sphdistance.py Outdated Show resolved Hide resolved
@willschlitzer
Copy link
Contributor Author

@seisman Doesn't sphdistance only require lat-long inputs? I don't think there should be z inputs.

@seisman seisman changed the title Make suggested fixes for sphdistance Expand table-like input options for sphdistance Sep 26, 2021
@seisman
Copy link
Member

seisman commented Sep 26, 2021

@seisman Doesn't sphdistance only require lat-long inputs? I don't think there should be z inputs.

I've never used sphdistance, but reading the sphdistance manpage, I think only x and y are required.

@willschlitzer
Copy link
Contributor Author

@seisman Doesn't sphdistance only require lat-long inputs? I don't think there should be z inputs.

I've never used sphdistance, but reading the sphdistance manpage, I think only x and y are required.

So should I then remove z from the parameters? I'm getting this error when I run the tests: pygmt.exceptions.GMTInvalidInput: data must provide x, y, and z columns

@seisman
Copy link
Member

seisman commented Sep 26, 2021

@seisman Doesn't sphdistance only require lat-long inputs? I don't think there should be z inputs.

I've never used sphdistance, but reading the sphdistance manpage, I think only x and y are required.

So should I then remove z from the parameters? I'm getting this error when I run the tests: pygmt.exceptions.GMTInvalidInput: data must provide x, y, and z columns

Yes, please. Sorry for misleading you.

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.

The docstring description reads: sphdistance reads one or more ASCII [or binary] files (or standard input), but 'standard input' (stdin) doesn't make sense in the Python world, suggest rewording it to use just 'table', similar to #1418 (comment).

@@ -22,7 +22,7 @@
V="verbose",
)
@kwargs_to_strings(I="sequence", R="sequence")
def sphdistance(table, **kwargs):
def sphdistance(data=None, x=None, y=None, **kwargs):
r"""
Create Voroni polygons from lat/lon coordinates.
Copy link
Member

@weiji14 weiji14 Sep 27, 2021

Choose a reason for hiding this comment

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

Typo here. Also, just noticed that this summary doesn't seem correct. It seems to suggest Voronoi polygons are created, but the output is a raster grid, not a vector polygon. Maybe follow https://docs.generic-mapping-tools.org/6.2/sphdistance.html.

Suggested change
Create Voroni polygons from lat/lon coordinates.
Create Voronoi polygons from lat/lon coordinates.

pygmt/src/sphdistance.py Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Sep 28, 2021

@willschlitzer The tests fail because the Windows server cannot download the cache file EGM96_to_36.txt, which was added in #1434, but not cached.

Please trigger the cache_data.yml workflow manually to update the caches.

@willschlitzer
Copy link
Contributor Author

Please trigger the cache_data.yml workflow manually to update the caches.

@seisman How do I do this?

@seisman
Copy link
Member

seisman commented Sep 28, 2021

Please trigger the cache_data.yml workflow manually to update the caches.

@seisman How do I do this?

You need to uncomment the following line (removing the leading #), commit the changes, and wait for the finish of the workflow. After that, you need to revert that commit:

.github/workflows/cache_data.yaml Outdated Show resolved Hide resolved
pygmt/src/sphdistance.py Outdated Show resolved Hide resolved
@willschlitzer willschlitzer merged commit e10df16 into main Sep 30, 2021
@willschlitzer willschlitzer deleted the sphdistance-fixes branch September 30, 2021 17:32
@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Oct 21, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…1491)

*Add parameter and tests for x/y inputs
*Rename "table" parameter to "data"
*Improve docstring for new parameters

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
Labels
enhancement Improving an existing feature skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants