-
Notifications
You must be signed in to change notification settings - Fork 224
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
Conversation
Co-authored-by: Yvonne Fröhlich <[email protected]>
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., |
|
Ah, I see. I just considered the "real" gallery examples and tutorials... |
I'm fine with removing |
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? |
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 |
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 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 |
Co-authored-by: Dongdong Tian <[email protected]> Co-authored-by: Yvonne Fröhlich <[email protected]>
OK, I think it's fine to keep this function unchanged.
Seismologists usually use ObsPy's get_events() function to get earthquakes from USGS or other seismic data centers. The function returns a |
Co-authored-by: Dongdong Tian <[email protected]>
@@ -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, |
There was a problem hiding this comment.
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.
This replaces
load_usgs_quakes
with the internal function_load_usgs_quakes
. It also adds additional checks totest_usgs_quakes
.Addresses: #2302
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version