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

Remove the deprecated load_usgs_quakes function (deprecated since v0.6.0) #2306

Merged
merged 11 commits into from
Jan 20, 2023

Conversation

willschlitzer
Copy link
Contributor

This replaces load_usgs_quakes with the internal function _load_usgs_quakes. It also adds additional checks to test_usgs_quakes.

Addresses: #2302

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.
  • Use underscores (not hyphens) in names of Python files and directories.

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

pygmt/datasets/samples.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Jan 9, 2023

I just realized that this sample dataset is not used in any examples. Should we remove it?

@yvonnefroehlich
Copy link
Member

I just realized that this sample dataset is not used in any examples. Should we remove it?

Hm. Not sure, as there seem to be other sample datasets which are currently also not used in any example, e.g., maunaloa_co2, hotspots, or mars_shape.

@seisman
Copy link
Member

seisman commented Jan 13, 2023

I just realized that this sample dataset is not used in any examples. Should we remove it?

Hm. Not sure, as there seem to be other sample datasets which are currently also not used in any example, e.g., maunaloa_co2, hotspots, or mars_shape.

  • mars_shape is used in pygmt/src/sphinterpolate.py,
  • maunaloa_co2 is used in pygmt/tests/test_filter1d.py
  • hotspots was added in Add function to import hotspot dataset #1386, and it seems there was a plan to use it in a sphdistance example.

@yvonnefroehlich
Copy link
Member

I just realized that this sample dataset is not used in any examples. Should we remove it?

Hm. Not sure, as there seem to be other sample datasets which are currently also not used in any example, e.g., maunaloa_co2, hotspots, or mars_shape.

  • mars_shape is used in pygmt/src/sphinterpolate.py,
  • maunaloa_co2 is used in pygmt/tests/test_filter1d.py
  • hotspots was added in Add function to import hotspot dataset #1386, and it seems there was a plan to use it in a sphdistance example.

Ah, I see. I just considered the "real" gallery examples and tutorials...

@willschlitzer
Copy link
Contributor Author

willschlitzer commented Jan 13, 2023

I'm fine with removing load_usgs_quakes. I don't remember it, but it looks like I planned to use it with sphdistance and that took a long time to merge, so I forgot about it.

@weiji14
Copy link
Member

weiji14 commented Jan 13, 2023

A quick search on GitHub shows it being used before at https://github.com/GenericMappingTools/try-gmt-python/blob/master/try-gmt-python.ipynb. But it's 4 years old now.

How much of a burden is it to keep the function maintained? Maybe we could just keep it in case?

@willschlitzer
Copy link
Contributor Author

It's not too much of an issue to keep it maintained. I just assumed that @seisman suggested removing it to avoid the precedent that all GMT remote files have an associated PyGMT function to load them.

@seisman
Copy link
Member

seisman commented Jan 14, 2023

It's not too much of an issue to keep it maintained. I just assumed that @seisman suggested removing it to avoid the precedent that all GMT remote files have an associated PyGMT function to load them.

I'm OK with maintaining this function, and I believe this dataset for global earthquakes can be used in examples in the future.

My major concern is that the returned data table contains too many columns and most of them are text strings and are unlikely used in any PyGMT examples. I suggest only keeping the "time", "latitude", "longitude", "depth" and "mag", similar to the data table returned by _load_japan_quakes.

@willschlitzer
Copy link
Contributor Author

willschlitzer commented Jan 14, 2023

It's not too much of an issue to keep it maintained. I just assumed that @seisman suggested removing it to avoid the precedent that all GMT remote files have an associated PyGMT function to load them.

I'm OK with maintaining this function, and I believe this dataset for global earthquakes can be used in examples in the future.

My major concern is that the returned data table contains too many columns and most of them are text strings and are unlikely used in any PyGMT examples. I suggest only keeping the "time", "latitude", "longitude", "depth" and "mag", similar to the data table returned by _load_japan_quakes.

I've never studied seismology, so I'm not sure how important information that pertains to the errors is to someone hoping to use this dataset. But I don't see the need to load this dataset with a separate function if we're going to remove quite a few of the columns. If it's the same columns as _load_japan_quakes, we might as well just use the Japan quakes dataset for any example/test functions. I don't feel strongly about removing this function, as someone may already be using it in their examples, but I don't wish to pare down the information in the returned dataset.

Looking at the USGS Earthquake API, we could probably create an example that incorporates getting a JSON file from the API and using it to create a plot/perform operations. I think that would be a better example of a real application of earthquake data instead of usgs_quakes_22.txt.

pygmt/datasets/samples.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Jan 15, 2023

It's not too much of an issue to keep it maintained. I just assumed that @seisman suggested removing it to avoid the precedent that all GMT remote files have an associated PyGMT function to load them.

I'm OK with maintaining this function, and I believe this dataset for global earthquakes can be used in examples in the future.
My major concern is that the returned data table contains too many columns and most of them are text strings and are unlikely used in any PyGMT examples. I suggest only keeping the "time", "latitude", "longitude", "depth" and "mag", similar to the data table returned by _load_japan_quakes.

I've never studied seismology, so I'm not sure how important information that pertains to the errors is to someone hoping to use this dataset. But I don't see the need to load this dataset with a separate function if we're going to remove quite a few of the columns. If it's the same columns as _load_japan_quakes, we might as well just use the Japan quakes dataset for any example/test functions. I don't feel strongly about removing this function, as someone may already be using it in their examples, but I don't wish to pare down the information in the returned dataset.

OK, I think it's fine to keep this function unchanged.

Looking at the USGS Earthquake API, we could probably create an example that incorporates getting a JSON file from the API and using it to create a plot/perform operations. I think that would be a better example of a real application of earthquake data instead of usgs_quakes_22.txt.

Seismologists usually use ObsPy's get_events() function to get earthquakes from USGS or other seismic data centers. The function returns a Catalog object which contains event information. This is related to the ObsPy integration in #967.

@@ -85,6 +85,7 @@ def load_sample_data(name):
"notre_dame_topography": _load_notre_dame_topography,
"ocean_ridge_points": _load_ocean_ridge_points,
"rock_compositions": _load_rock_sample_compositions,
"usgs_quakes": _load_usgs_quakes,
Copy link
Member

Choose a reason for hiding this comment

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

Need to remove "usgs_quakes": load_usgs_quakes at line 75.

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Jan 20, 2023
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Jan 20, 2023
@seisman seisman merged commit 03f66b1 into main Jan 20, 2023
@seisman seisman deleted the deprecate-load_usgs_quakes branch January 20, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Deprecating a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants