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

MD: Let 'channel' and 'location' override the priority lists #2047

Merged
merged 4 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@paride
Member

paride commented Jan 12, 2018

What does this PR do?

This makes the code works as documented, that is: if the channel variable is set, then the channel_priorities list is ignored. The same goes for the location variable and location_priorities.

Why was it initiated? Any relevant Issues?

Fixes #1810.
Fixes #2031.

PR Checklist

  • Correct base branch selected? master for new fetures, 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 .
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 13, 2018

Member

Travis.. hip hip hooray 🎉

Member

megies commented Jan 13, 2018

Travis.. hip hip hooray 🎉

@megies megies added this to the 1.1.1 milestone Jan 13, 2018

@krischer

krischer requested changes Jan 15, 2018 edited

Sorry for taking so long to review this. This looks pretty good. Can you add a short test? You can look at one of the existing mock tests to use as a template and you would basically have to check if utils.filter_channel_priority() is being called with various inputs.

@paride

This comment has been minimized.

Show comment
Hide comment
@paride

paride Jan 23, 2018

Member

@krischer Sorry for the late reply, for some reason I didn't get (or I accidentally deleted) the email notification for your message. I'll try to write a tests ASAP, as I'd really like this patch to make it into v1.1.1.

Member

paride commented Jan 23, 2018

@krischer Sorry for the late reply, for some reason I didn't get (or I accidentally deleted) the email notification for your message. I'll try to write a tests ASAP, as I'd really like this patch to make it into v1.1.1.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 24, 2018

Member

@paride please also rebase on current maintenance branch, that should get rid of unrelated test fails in CI

Member

megies commented Jan 24, 2018

@paride please also rebase on current maintenance branch, that should get rid of unrelated test fails in CI

@paride

This comment has been minimized.

Show comment
Hide comment
@paride
Member

paride commented Jan 25, 2018

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 25, 2018

Member

Yes please add something more specific. Sorry for being paranoid but the mass downloader code is already super complex and I would not feel comfortable adding new untested features. A new test should best be written in a way that it would fail without your changes - then we can be fairly confident that we don't reintroduce the issue with future updates.

A good starting point for a test would be this chunk of code here:

def test_excluding_networks_and_stations(self):
"""
Tests the excluding of networks and stations.
"""
# Default
c = self._init_client()
c.client.get_stations.return_value = obspy.read_inventory(
os.path.join(self.data, "channel_level_fdsn.txt"))
c.get_availability()
self.assertEqual([("AK", "BAGL"), ("AK", "BWN"), ("AZ", "BZN")],
sorted(c.stations.keys()))

In the new test you could modify the location and channel attributes and test the effects on the availability query. Note that c.stations also have values in case you need more details for the test.

Member

krischer commented Jan 25, 2018

Yes please add something more specific. Sorry for being paranoid but the mass downloader code is already super complex and I would not feel comfortable adding new untested features. A new test should best be written in a way that it would fail without your changes - then we can be fairly confident that we don't reintroduce the issue with future updates.

A good starting point for a test would be this chunk of code here:

def test_excluding_networks_and_stations(self):
"""
Tests the excluding of networks and stations.
"""
# Default
c = self._init_client()
c.client.get_stations.return_value = obspy.read_inventory(
os.path.join(self.data, "channel_level_fdsn.txt"))
c.get_availability()
self.assertEqual([("AK", "BAGL"), ("AK", "BWN"), ("AZ", "BZN")],
sorted(c.stations.keys()))

In the new test you could modify the location and channel attributes and test the effects on the availability query. Note that c.stations also have values in case you need more details for the test.

@paride

This comment has been minimized.

Show comment
Hide comment
@paride

paride Jan 27, 2018

Member

@krischer I added a simple test that fails without my changes, let me know what you think. If you prefer something to be done differently or somewhere else just tell me.

Member

paride commented Jan 27, 2018

@krischer I added a simple test that fails without my changes, let me know what you think. If you prefer something to be done differently or somewhere else just tell me.

@paride

This comment has been minimized.

Show comment
Hide comment
@paride

paride Jan 28, 2018

Member

@krischer I could have made the test differently: changing the priorities to something uncommon, while setting location and channel to something that is already available in channel_level_fdsn.txt. In this way it is not necessary to add the new uncommon_channel_location.txt. Let me know if you prefer it to be done in this way.

Member

paride commented Jan 28, 2018

@krischer I could have made the test differently: changing the priorities to something uncommon, while setting location and channel to something that is already available in channel_level_fdsn.txt. In this way it is not necessary to add the new uncommon_channel_location.txt. Let me know if you prefer it to be done in this way.

paride and others added some commits Jan 12, 2018

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 29, 2018

Member

@paride Thanks a lot!

I added a couple more scenarios to the test case just to be safe. I also had to rebase to get rid of a changelog config so make sure you do a hard reset if you continue working on this branch.

Can be merged from my point of view if CI is clean.

+TESTS: clients.fdsn

Member

krischer commented Jan 29, 2018

@paride Thanks a lot!

I added a couple more scenarios to the test case just to be safe. I also had to rebase to get rid of a changelog config so make sure you do a hard reset if you continue working on this branch.

Can be merged from my point of view if CI is clean.

+TESTS: clients.fdsn

@krischer krischer merged commit 533c7a1 into maintenance_1.1.x Jan 30, 2018

5 of 6 checks passed

docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 88.45% (+1.52%) compared to 18d33d2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer deleted the MD-honor-channel-and-location branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment