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

Geographic selection of inventory/network/station #2515

Open
wants to merge 2 commits into
base: master
from

Conversation

@claudiodsf
Copy link
Member

claudiodsf commented Dec 17, 2019

What does this PR do?

Add geographic options to [Inventory,Network,Station].select(), following
the same syntax of Client.get_stations().

Also add documentation to "sampling_rate" parameter.

Why was it initiated? Any relevant Issues?

Sometimes datacenter provides preassembled datasets with data and metadata for all the stations in the network (ex.: http://cnt.rm.ingv.it/event/23558121 --> Download). This PR adds possibility of filtering metadata based on geographic parameters, following the same syntax of Client.get_stations().
Not covered by this PR is the possibility of selecting data in stream based on the inventory content (next time ;-)

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:"
    (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 .
@claudiodsf claudiodsf force-pushed the claudiodsf:inventory_select branch from 31cf3bb to 0c86979 Dec 17, 2019
@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Dec 17, 2019

Fixed a typo in documentation and force-pushed.

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Dec 18, 2019

would it make sense to put the location-based filtering in some generic util module to avoid the large copy/paste of identical codes ?

obspy/core/inventory/network.py Outdated Show resolved Hide resolved
@megies
megies approved these changes Dec 18, 2019
Copy link
Member

megies left a comment

  • please move keep_empty to old position
  • please add a changelog one-liner
  • CircleCI fails are unrelated

Otherwise good to go, looks like (didn't check tests in detail).

@megies megies added this to the 1.2.0 milestone Dec 18, 2019
@megies megies added this to In Progress in Release 1.2.0 Dec 18, 2019
Copy link
Member Author

claudiodsf left a comment

would it make sense to put the location-based filtering in some generic util module to avoid the large copy/paste of identical codes ?

I thought about it. Where would you put this generic code?

obspy/core/inventory/inventory.py Outdated Show resolved Hide resolved
@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Dec 18, 2019

would it make sense to put the location-based filtering in some generic util module to avoid the large copy/paste of identical codes ?

I thought about it. Where would you put this generic code?

Yeah, not easy indeed, and the way you skip the channel is indeed not so easily "generalizable"...

maybe something like this:

def _filter_object_geographically(object, min_latitude, max_latitude, ...):
    if minlatitude is not None:
        if obj.latitude is None or obj.latitude < minlatitude:
            return False
    if maxlatitude is not None:
        if obj.latitude is None or obj.latitude > maxlatitude:
            return False
    if minlongitude is not None:
        if obj.longitude is None or obj.longitude < minlongitude:
            return False
    if maxlongitude is not None:
        if obj.longitude is None or obj.longitude > maxlongitude:
            return False
    if all([l is not None for l in
           (latitude, longitude, obj.latitude, obj.longitude)]):
        distance = locations2degrees(latitude, longitude,
                                     obj.latitude, obj.longitude)
        if minradius is not None and distance < minradius:
            return False
        if maxradius is not None and distance > maxradius:
            return False
    return True

and then in your calls:

for cha in self.channels:
    if _filter_object_geographically(cha, min_latitude, max_latitude, ...):
        channels.append(cha)

and

for sta in self.stations:
    if not _filter_object_geographically(sta, min_latitude, max_latitude, ...):
        continue
@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 18, 2019

would it make sense to put the location-based filtering in some generic util module to avoid the large copy/paste of identical codes ?

avoiding code duplication is always good 👍

@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Dec 18, 2019

would it make sense to put the location-based filtering in some generic util module to avoid the large copy/paste of identical codes ?

avoiding code duplication is always good 👍

Ok, should I put then this common code in core/inventory/util.py ?

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 18, 2019

Ok, should I put then this common code in core/inventory/util.py ?

I guess it could be useful for events/catalog as well? Then it should go into core/util/misc.py or maybe better geodetics/base.py (the latter might create a circular import though, didnt check)

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Dec 18, 2019

actually, I think it could be in geodetics/base.py and be a public function, as long as an object exposes the latitude and longitude parameters, it could be used.

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 18, 2019

Yes, agree. If it causes a circular import we see it in CI

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 19, 2019

It might be a good idea to add a test that this works across hemispheres - e.g. in New Zealand we have stations both sides of 180W and have run into issues in the past with simple minlatitude and maxlatitude not wrapping the search around +/- 180 (GeoNet fixed this issue here).

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 19, 2019

We had some issue with wrapping around longitude +-180° at some point, I was pretty sure, but I can't find it now..

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Dec 20, 2019

@claudiodsf while you are at it, maybe think about making the selection inclusive or not( > or >= etc.) ?

@claudiodsf claudiodsf force-pushed the claudiodsf:inventory_select branch 2 times, most recently from 6998bf7 to 97145b8 Jan 9, 2020
@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 9, 2020

Finally found some time to work on this. Let me know what do you think.
I'll add a CHANGELOG entry before merging 😉

@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 9, 2020

@claudiodsf while you are at it, maybe think about making the selection inclusive or not( > or >= etc.) ?

Not sure whether this is essential: geographic coordinates are never that precise to require strict equality. And that would make the code and the function interface more complex.

Edit: also equality of floating numbers makes little sense to me.

@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 10, 2020

Not sure which kind of errors the docs builds reports...

@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 10, 2020

Also, feel free to propose a better name for filter_object_geographically().
It is more a check than a filter : check that an object is within a certain geographic range.

@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 10, 2020

Possible alternative names:

object_within_geographic_bounds()
object_inside_geographic_bounds()
object_within_geographic_range()
object_inside_geographic_range()

Other ideas?

@claudiodsf claudiodsf mentioned this pull request Jan 12, 2020
5 of 7 tasks complete
@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Jan 13, 2020

inside_geobounds ? :-)

@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 13, 2020

inside_geobounds ? :-)

@megies what do you think?

@megies

This comment has been minimized.

Copy link
Member

megies commented Jan 15, 2020

Works for me

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Jan 20, 2020

@claudiodsf please make the name change and I'll pass for another (hopefully last) review of the code.

claudiodsf added 2 commits Dec 17, 2019
Add geographic options to [Inventory,Network,Station].select(), following
the same syntax of Client.get_stations().

Also add documentation to "sampling_rate" parameter.
Utility function to check whether an object is within a given
latitude and/or longitude range, or within a given distance range
from a reference geographic point.
@claudiodsf claudiodsf force-pushed the claudiodsf:inventory_select branch from ee26cd9 to 860ed8d Jan 20, 2020
@claudiodsf

This comment has been minimized.

Copy link
Member Author

claudiodsf commented Jan 20, 2020

@claudiodsf please make the name change and I'll pass for another (hopefully last) review of the code.

@ThomasLecocq Done! Let's wait on CI tests, then please review!

@ThomasLecocq ThomasLecocq moved this from In Progress to Waiting for Review in Release 1.2.0 Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release 1.2.0
Waiting for Review
4 participants
You can’t perform that action at this time.