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 grdfill #1276

Merged
merged 11 commits into from
May 21, 2021
Merged

Wrap grdfill #1276

merged 11 commits into from
May 21, 2021

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented May 17, 2021

This pull request wraps the grdfill module.

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 17, 2021
@willschlitzer willschlitzer added this to the 0.4.0 milestone May 17, 2021
@willschlitzer willschlitzer self-assigned this May 17, 2021
@willschlitzer
Copy link
Contributor Author

@GenericMappingTools/pygmt-contributors Any ideas for how to test this? I have downloaded grid files from scientific cruises to ensure I have gaps in the data that need to be filled, but I'm not sure how to write a test that retrieves an unfilled grid from something like pygmt.datasets.

@maxrjones
Copy link
Member

@GenericMappingTools/pygmt-contributors Any ideas for how to test this? I have downloaded grid files from scientific cruises to ensure I have gaps in the data that need to be filled, but I'm not sure how to write a test that retrieves an unfilled grid from something like pygmt.datasets.

You can manual set one or more of the data values in grids from pygmt.datasets to NaN to test the result from grdfill. For example:

import numpy as np
import pytest
import xarray as xr
from pygmt import grdfill
from pygmt.datasets import load_earth_relief


@pytest.fixture(scope="module", name="grid")
def fixture_grid():
    """
    Load the grid data from the sample earth_relief file and set value(s) to NaN.
    """
    grid = load_earth_relief(registration="pixel")
    grid[10,10] = np.nan
    return grid

def test_grdfilter_dataarray_in_dataarray_out(grid):
    """
    grdfill an input DataArray, and output as DataArray.
    """
    result = grdfill(grid=grid,mode="c20")
    # check information of the output grid
    assert isinstance(result, xr.DataArray)
    assert result[10,10] == 20

@willschlitzer
Copy link
Contributor Author

You can manual set one or more of the data values in grids from pygmt.datasets to NaN to test the result from grdfill. For example:

Thanks @meghanrjones!

@willschlitzer willschlitzer changed the title WIP: Wrap grdfill Wrap grdfill May 18, 2021
@willschlitzer willschlitzer marked this pull request as ready for review May 18, 2021 08:04
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.

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label May 19, 2021
Copy link
Member

@michaelgrund michaelgrund 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. Great work!

Copy link
Member

@core-man core-man left a comment

Choose a reason for hiding this comment

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

Looks great. Only two minor comments.

pygmt/src/grdfill.py Outdated Show resolved Hide resolved
Specify the hole-filling algorithm to use. Choose from **c** for
constant fill and append the constant value, **n** for nearest
neighbor (and optionally append a search radius in
pixels [default radius is `r^2 = sqrt(X^2 + Y^2)`,
Copy link
Member

@core-man core-man May 19, 2021

Choose a reason for hiding this comment

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

LaTeX formula?

Suggested change
pixels [default radius is `r^2 = sqrt(X^2 + Y^2)`,
pixels [default radius is :math:`r^2 = \sqrt{X^2 + Y^2}`,

Copy link
Member

Choose a reason for hiding this comment

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

If you want to use the math directive, it should be:

:math:`r^2 = \sqrt{X^2 + Y^2}`

Copy link
Member

Choose a reason for hiding this comment

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

If you want to use the math directive, it should be:

:math:`r^2 = \sqrt{X^2 + Y^2}`

updated~ Thanks

Copy link
Member

Choose a reason for hiding this comment

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

@willschlitzer previously had this in there - it did not work because fmt_docstring tries to insert common text into the brackets. So, it either needs to stay as non-sphinx math or fmt_docstring needs to be modified.

Copy link
Member

@seisman seisman May 19, 2021

Choose a reason for hiding this comment

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

If we want LaTeX math, we may need to use double curly braces

:math:`r^2 = \sqrt{{ X^2 + Y^2 }}`

See https://stackoverflow.com/questions/5466451

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want LaTeX math, we may need to use double curly braces

:math:`r^2 = \sqrt{{ X^2 + Y^2 }}`

See https://stackoverflow.com/questions/5466451

This appears to be working!

@maxrjones
Copy link
Member

What is the preferred was for denoting required parameters? I think there should be some indication in the method docs that mode (-A) is required.

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.

Just some suggestions to improve the unit tests (smaller grid for speed + more robust checks) 😁 I've also thrown in some np.inf values just to see if GMT would fill them in.

What is the preferred was for denoting required parameters? I think there should be some indication in the method docs that mode (-A) is required.

Perhaps just a one-line description in the docstring wll suffice?

pygmt/tests/test_grdfill.py Outdated Show resolved Hide resolved
pygmt/tests/test_grdfill.py Outdated Show resolved Hide resolved
pygmt/tests/test_grdfill.py Outdated Show resolved Hide resolved
Co-authored-by: Yao Jiayuan <coreman.seism@gmail.com>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@willschlitzer willschlitzer removed the final review call This PR requires final review and approval from a second reviewer label May 21, 2021
@willschlitzer willschlitzer merged commit cc26838 into master May 21, 2021
@willschlitzer willschlitzer deleted the wrap-grdfill branch May 21, 2021 09:56
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Wrap the gmt module grdfill and create test_grdfill.py.

Co-authored-by: Yao Jiayuan <coreman.seism@gmail.com>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
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