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

Add earth_relief_holes to load_sample_data() #1921

Merged
merged 13 commits into from
May 25, 2022

Conversation

willschlitzer
Copy link
Contributor

This adds a function to load the remote dataset @earth_relief_20m_holes.grd for use with the in-line example for grdfill.

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 wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • 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 May 10, 2022
@willschlitzer willschlitzer self-assigned this May 10, 2022
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 @willschlitzer! Do you also want to add @earth_relief_20m_holes to the cache list at

# List of datasets to download
datasets = [
# Earth relief grids
?

pygmt/tests/test_datasets_samples.py Outdated Show resolved Hide resolved
pygmt/tests/test_datasets_samples.py Outdated Show resolved Hide resolved
pygmt/tests/test_datasets_samples.py Outdated Show resolved Hide resolved
willschlitzer and others added 2 commits May 10, 2022 20:59
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@willschlitzer willschlitzer changed the title Add a earth_relief_holes to load_sample_data() Add earth_relief_holes to load_sample_data() May 10, 2022
@seisman seisman added this to the 0.7.0 milestone May 11, 2022
pygmt/datasets/samples.py Outdated Show resolved Hide resolved
pygmt/datasets/samples.py Outdated Show resolved Hide resolved
.github/workflows/cache_data.yaml Outdated Show resolved Hide resolved
@seisman seisman self-requested a review May 11, 2022 04:31
@@ -343,3 +346,18 @@ def load_mars_shape(**kwargs):
fname, sep="\t", header=None, names=["longitude", "latitude", "radius(m)"]
)
return data


def _load_earth_relief_holes(**kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Pylint reports the following warning.

pygmt/datasets/samples.py:351:0: W0613: Unused argument 'kwargs' (unused-argument)

I think we don't need kwagrs for new datasets.

Suggested change
def _load_earth_relief_holes(**kwargs):
def _load_earth_relief_holes():

Copy link
Contributor Author

@willschlitzer willschlitzer May 23, 2022

Choose a reason for hiding this comment

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

I'll check again to confirm after I rewrite the tests, but I think it needs **kwargs because of data = load_func[name](suppress_warning=True). It was getting passed a value for suppress_warning but wasn't expecting any arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I received TypeError: _load_earth_relief_holes() got an unexpected keyword argument 'suppress_warning' when I tried running it without **kwargs.

Copy link
Member

@seisman seisman May 24, 2022

Choose a reason for hiding this comment

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

The suppress_warning parameter (or **kwargs) was added so that users who call the deprecated, old load_xxx functions can silent the deprecation warnings by passing suppress_warning=True. Thus, **kwargs is NOT required for new private _load_xxx functions.

Do you think we should rewrite the code to call the old and new load_xxx functions in different ways:

    # old public load functions for backward compatibility
    load_func_old = {
        "bathymetry": load_sample_bathymetry,
        "fractures": load_fractures_compilation,
        "hotspots": load_hotspots,
        "japan_quakes": load_japan_quakes,
        "mars_shape": load_mars_shape,
        "ocean_ridge_points": load_ocean_ridge_points,
        "usgs_quakes": load_usgs_quakes,
    }
    # new private load functions
    load_func = {
        "earth_relief_holes": _load_earth_relief_holes,
    }
    if name in load_func_old:
        data = load_func[name](suppress_warning=True)
    else:
        data = load_func[name]
    return data

Copy link
Contributor Author

@willschlitzer willschlitzer May 24, 2022

Choose a reason for hiding this comment

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

I think that's a good idea. I think this should be a separate PR; does that work? I'm happy to do it (admittedly copying the code above) but won't be able to get to it until tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

A separate PR is OK to me.

@willschlitzer
Copy link
Contributor Author

@GenericMappingTools/pygmt-admin It looks like this is failing some tests because of the change to gridline registration for @earth_relief_20m_holes.grd. Is this file now stable?

@seisman
Copy link
Member

seisman commented May 23, 2022

@GenericMappingTools/pygmt-admin It looks like this is failing some tests because of the change to gridline registration for @earth_relief_20m_holes.grd. Is this file now stable?

xref: the grid file registration was changed in GenericMappingTools/gmtserver-admin#156

@maxrjones
Copy link
Member

@GenericMappingTools/pygmt-admin It looks like this is failing some tests because of the change to gridline registration for @earth_relief_20m_holes.grd. Is this file now stable?

Yes, it's now stable so the baseline images can be updated.

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label May 24, 2022
@seisman seisman merged commit 8ddd11c into main May 25, 2022
@seisman seisman deleted the add-dataset/earth-relief-holes branch May 25, 2022 13:40
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label May 25, 2022
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants