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

adding RASPISHAKE to FDSN mapping #2555

Merged
merged 10 commits into from
Feb 14, 2020
Merged

adding RASPISHAKE to FDSN mapping #2555

merged 10 commits into from
Feb 14, 2020

Conversation

iannesbitt
Copy link
Contributor

@iannesbitt iannesbitt commented Feb 12, 2020

What does this PR do?

This adds the base of the Raspberry Shake FDSN tree (http://fdsnws.raspberryshakedata.com) to the URL mapping list under "RSHAKE" so that it can be accessed via

client = Client("RSHAKE")

Why was it initiated? Any relevant Issues?

This is being initiated to add the RASPISHAKE datacenter (DOI: 10.7914/SN/AM) to the list of URL mapping endpoints.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:clients.fdsn"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

@iannesbitt iannesbitt marked this pull request as ready for review February 13, 2020 00:00
@iannesbitt
Copy link
Contributor Author

It looks like (relevant) tests have passed, so updating PR checkbox above.

@krischer
Copy link
Member

One thing to consider is if we should exclude this special data center from the default data centers in the mass downloader (https://docs.obspy.org/packages/autogen/obspy.clients.fdsn.mass_downloader.html).

It could still be included if users choose to do so but I think this should be an explicit choice given the non-standard nature of this particular data center. It might also hit it with a lot of potentially unwanted requests.

@megies
Copy link
Member

megies commented Feb 13, 2020

It could still be included if users choose to do so but I think this should be an explicit choice given the non-standard nature of this particular data center. It might also hit it with a lot of potentially unwanted requests.

👍

Also, I'd vote for RASPISHAKE

  • RSHAKE is not an (semi)official abbreviation as far as I know
  • it's way clearer
  • only 3 chars longer
  • it's the official Twitter handle
  • it's the name in the FDSN datacenter list

@megies megies added this to the 1.2.0 milestone Feb 13, 2020
@iannesbitt
Copy link
Contributor Author

Also, I'd vote for RASPISHAKE

I had considered this but decided against it because it was longer than any others, and required a reformatting of the tab alignment on the docs page. I agree that it fits better with the datacenter's other identifiers.

@iannesbitt
Copy link
Contributor Author

One thing to consider is if we should exclude this special data center from the default data centers in the mass downloader

@krischer disabled by default would be best I think.

@iannesbitt
Copy link
Contributor Author

@krischer am I correct in assuming that the exclusion for RASPISHAKE is done in the init function of the MassDownloader class, i.e. https://docs.obspy.org/_modules/obspy/clients/fdsn/mass_downloader/mass_downloader.html#MassDownloader.__init__?

@megies
Copy link
Member

megies commented Feb 13, 2020

Looks like it. Also please add a note in the docstring for providers that raspishake is omitted by default and has to be specified/added explicitly.
I guess one would have to do e.g. something like the following to get raspishake in there

from obspy.clients.fdsn import URL_MAPPINGS
MassDownloader(providers=URL_MAPPINGS)

@iannesbitt
Copy link
Contributor Author

iannesbitt commented Feb 13, 2020

@megies ok, consider these changes:

class MassDownloader(object):
    """
    Class facilitating data acquisition across all FDSN web service
    implementations.

    :param providers: List of FDSN client names or service URLS. Will use
        all FDSN implementations known to ObsPy except RASPISHAKE if set to
        None. The order in the list also determines their priority, if data
        is available at more then one provider it will always be downloaded
        from the provider that comes first in the list. To include RASPISHAKE,
        you must set this parameter to
        `obspy.clients.fdsn.header.URL_MAPPINGS` explicitly.
    :param debug: Debug flag passed to the underlying FDSN web service clients.
    :type providers: list of str or :class:`~obspy.clients.fdsn.client.Client`
        instances
    """
    def __init__(self, providers=None, debug=False):
# ...
        if providers is None:
            providers = dict(URL_MAPPINGS.items())
# ...
            if "RASPISHAKE" in providers:
                # exclude RASPISHAKE by default
                del providers["RASPISHAKE"]
# ...

@iannesbitt iannesbitt changed the title adding RSHAKE to FDSN mapping adding RASPISHAKE to FDSN mapping Feb 13, 2020
@megies
Copy link
Member

megies commented Feb 14, 2020

This needs to be adapted: https://travis-ci.org/obspy/obspy/jobs/650081061#L1438

Also some trailing whitespace.

@megies
Copy link
Member

megies commented Feb 14, 2020

@iannesbitt just for future reference, you should work on branches in your fork, your master is now out of sync with the main repo master branch.

@megies megies merged commit 9bc822d into obspy:master Feb 14, 2020
@iannesbitt
Copy link
Contributor Author

@iannesbitt just for future reference, you should work on branches in your fork, your master is now out of sync with the main repo master branch.

Ok. Noted. Thanks for the quick responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants