-
Notifications
You must be signed in to change notification settings - Fork 529
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
add Inventory.sort() #2087
base: master
Are you sure you want to change the base?
add Inventory.sort() #2087
Conversation
@Dominikstr this could be something you can work on. |
I have a few questions regarding this one. What exactly is the aim of the function: I'm not completely sure if it's possible to return an inventory object, which is sorted by sample_rate for example. The only way I could think of would be multiple network objects with the same name. These network objects contain all the stations/channels with the same value of the search criteria and are ordered accordingly. The example looks like the aim is to return a list of channels but I'm not completely sure. |
I guess we could maybe start by having an |
This commit contains the first try for a sorting function. Until now its only a simple sort first by code and then by start_date.
This reverts commit 162a0d7.
Basic sorting function sorting first by code and then by starting date.
I committed the first try on a basic sorting function. It would be pretty easy to make the sorting parameters adjustable, so I could add that if needed. If this is the direction you want I will write a test case, description and make sure it's stable. |
@Dominikstr please push your commit to a differently named branch. Only alphanumeric characters, dashes, underscores please. Brackets in the branch name are real ugly and a mess to work with on command line.. I'll make this issue a PR then for better review options I can't even open you branch in my browser because it hickups with the round brackets.. |
I renamed the branch, hopefully it is working. |
Small change to only sort in place plus docstring extension.
@Dominikstr looks like github changed the issue->PR conversion API. I can't convert this issue into a PR because I don't have write access on your fork. So I guess, you can just open a separate PR (or add me as collaborator on your fork). |
I added you as a collaborator and I will open the pull request as soon as I finish the test case. |
No need, I made this issue into a PR now. It will dynamically update whenever you push to your branch again. :-) |
Contains a test case for the sorting method and to new inventory files. One to sort and one to compare.
Instead of basing the test case on two StationXML files it would probably be better to just create some dummy Inventory objects and perform the test on these. Avoids having to commit test files but also is IMHO easier to reason about, especially for different cases like reverse sorting. |
The new test is a bit long but I tried to keep it as short as possible. I couldn't think of a way to make it more compact. |
Basic sorting function sorting first by code and then by starting date.
Contains a test case for the sorting method and to new inventory files. One to sort and one to compare.
Basic sorting function sorting first by code and then by starting date.
Contains a test case for the sorting method and to new inventory files. One to sort and one to compare.
…d_2087_Inventory_sort # Conflicts: # obspy/core/tests/test_inventory.py
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.
One long in-line comment with a few suggestions.
Also I think it would be good to replace the test files with some generated test objects (e.g. manually create some inventory/network/... objects and run the tests on them).
obspy/core/inventory/inventory.py
Outdated
@@ -1117,6 +1117,27 @@ def plot_response(self, min_freq, output="VEL", network="*", station="*", | |||
|
|||
return fig | |||
|
|||
def sort(self): |
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.
How about something like this:
def sort(self): | |
def sort(self, keys=("code", "start_date"), max_level="channel"): | |
sort_fct = lambda x: [getattr(x, i) for i in keys] | |
self.networks = sorted(self.network, key=sort_fct) | |
... |
Then it works for all keys and any number of keys (at least those that are present on all object types which could also checked before any in-place sorting takes place). I also added a max_level
argument so one could for example only sort the networks and not the stations/channels/...
I also think the .sort()
method should be part of every network/station/... object. If you implement it at the BaseNode
(https://github.com/obspy/obspy/blob/master/obspy/core/inventory/util.py#L26) this would just work I think.
… a sorting function for networks and stations at the base node.
I implemented the sort at the base node and changed the inventory sort according to your suggested change. If this is what you have in mind I will add a few lines to deal with wrong input and test cases for the base node implementation. I'm still not entirely sure what you mean by manually creating the objects. Something like commit b3d7b13 ? |
… a sorting function for networks and stations at the base node.
…d_2087_Inventory_sort # Conflicts: # obspy/core/inventory/inventory.py
It might be useful to have a sorting method on
Inventory
, something like..Looking at the contents of
inv
before sorting:..and after:
We could offer to sort by epoch start or end time or also sampling rate etc..