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

Make Stream.rotate() take an Inventory object / Stream.rotate from unaligned to ZNE #1310

Merged
merged 20 commits into from Oct 8, 2017

Conversation

@megies
Member

megies commented Mar 4, 2016

We should think about enhancing the Stream.rotate() function to be able to feed it an inventory object (with included dip/azimuth of channels).

TODO:

  • check method naming / new method / refactor existing method
  • add test
  • changelog
  • Tutorial (we don't have a rotation tutorial right now..) / API docs example usage

@megies megies added this to the 1.1.0 milestone Mar 2, 2016

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 2, 2016

Member

👍

This would likely require a different interface. Something like.

Stream.rotate(inventory=inv, rotate_to="ZNE")

could work without breaking backwards compatibility.

Member

krischer commented Mar 2, 2016

👍

This would likely require a different interface. Something like.

Stream.rotate(inventory=inv, rotate_to="ZNE")

could work without breaking backwards compatibility.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 2, 2016

Member

Stream.rotate(inventory=inv, rotate_to="ZNE")
could work without breaking backwards compatibility.

how would that not break the current
Stream.rotate(method, back_azimuth=None, inclination=None) ?

Member

megies commented Mar 2, 2016

Stream.rotate(inventory=inv, rotate_to="ZNE")
could work without breaking backwards compatibility.

how would that not break the current
Stream.rotate(method, back_azimuth=None, inclination=None) ?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 2, 2016

Member

method would of course have to default to None. But then all old calls should still work. It would be a bit awkward but it should be possible to not break old code.

Member

krischer commented Mar 2, 2016

method would of course have to default to None. But then all old calls should still work. It would be a bit awkward but it should be possible to not break old code.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 2, 2016

Member

method would of course have to default to None. But then all old calls should still work. It would be a bit awkward but it should be possible to not break old code.

I guess you're assuming people use it with explicit rotate(method="xxx") as opposed to rotate("xxx")? Unfortunately, I don't think we can assume this..

Member

megies commented Mar 2, 2016

method would of course have to default to None. But then all old calls should still work. It would be a bit awkward but it should be possible to not break old code.

I guess you're assuming people use it with explicit rotate(method="xxx") as opposed to rotate("xxx")? Unfortunately, I don't think we can assume this..

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 2, 2016

Member

Sorry for being unclear. The current signature is

Stream.rotate(method, back_azimuth=None, inclination=None)

and we just change it to

Stream.rotate(method=None, back_azimuth=None, inclination=None, inventory=None, rotate_to=None)

All old calls will still work we just have to implement some akward argument parsing logic. After 1.0 we probably cannot change the call signature of such a common function again :-(

Alternatively we could introduce a new method Stream.rotate_to(). I personally favor the first alternative.

Member

krischer commented Mar 2, 2016

Sorry for being unclear. The current signature is

Stream.rotate(method, back_azimuth=None, inclination=None)

and we just change it to

Stream.rotate(method=None, back_azimuth=None, inclination=None, inventory=None, rotate_to=None)

All old calls will still work we just have to implement some akward argument parsing logic. After 1.0 we probably cannot change the call signature of such a common function again :-(

Alternatively we could introduce a new method Stream.rotate_to(). I personally favor the first alternative.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 4, 2016

Member

I've started some work on this, for now mainly on preparing data for rotation.. (cutting channels down to common time slices).

from obspy import read

st = read()
t1 = st[0].stats.starttime
t2 = st[0].stats.endtime
delta = st[0].stats.delta

tr = st[0]
tr.stats.starttime += 4 * delta
tr = st[1]
tr.data = tr.data[:-13]
tr = st[2]
tr.stats.starttime -= 7 * delta
tr.data = tr.data[:-13]

st_ = st.select(channel="EHZ").copy()
st.pop(0)
st += st_.cutout(t1+3, t1+9)
st_ = st.select(channel="EHN").copy()
st.pop(0)
st += st_.cutout(t1+5, t1+18)
st_ = st.select(channel="EHE").copy()
st.pop(0)
st += st_.cutout(t1+23, t1+25)

print(st)
st._trim_common_channels()
print(st)
6 Trace(s) in Stream:
BW.RJOB..EHZ | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHZ | 2009-08-24T00:20:12.000000Z - 2009-08-24T00:20:33.030000Z | 100.0 Hz, 2104 samples
BW.RJOB..EHN | 2009-08-24T00:20:03.000000Z - 2009-08-24T00:20:08.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHN | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:32.860000Z | 100.0 Hz, 1187 samples
BW.RJOB..EHE | 2009-08-24T00:20:02.930000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 2308 samples
BW.RJOB..EHE | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
9 Trace(s) in Stream:
BW.RJOB..EHZ | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHN | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHE | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHZ | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHN | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHE | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHZ | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
BW.RJOB..EHN | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
BW.RJOB..EHE | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
Member

megies commented Mar 4, 2016

I've started some work on this, for now mainly on preparing data for rotation.. (cutting channels down to common time slices).

from obspy import read

st = read()
t1 = st[0].stats.starttime
t2 = st[0].stats.endtime
delta = st[0].stats.delta

tr = st[0]
tr.stats.starttime += 4 * delta
tr = st[1]
tr.data = tr.data[:-13]
tr = st[2]
tr.stats.starttime -= 7 * delta
tr.data = tr.data[:-13]

st_ = st.select(channel="EHZ").copy()
st.pop(0)
st += st_.cutout(t1+3, t1+9)
st_ = st.select(channel="EHN").copy()
st.pop(0)
st += st_.cutout(t1+5, t1+18)
st_ = st.select(channel="EHE").copy()
st.pop(0)
st += st_.cutout(t1+23, t1+25)

print(st)
st._trim_common_channels()
print(st)
6 Trace(s) in Stream:
BW.RJOB..EHZ | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHZ | 2009-08-24T00:20:12.000000Z - 2009-08-24T00:20:33.030000Z | 100.0 Hz, 2104 samples
BW.RJOB..EHN | 2009-08-24T00:20:03.000000Z - 2009-08-24T00:20:08.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHN | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:32.860000Z | 100.0 Hz, 1187 samples
BW.RJOB..EHE | 2009-08-24T00:20:02.930000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 2308 samples
BW.RJOB..EHE | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
9 Trace(s) in Stream:
BW.RJOB..EHZ | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHN | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHE | 2009-08-24T00:20:03.040000Z - 2009-08-24T00:20:06.000000Z | 100.0 Hz, 297 samples
BW.RJOB..EHZ | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHN | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHE | 2009-08-24T00:20:21.000000Z - 2009-08-24T00:20:26.000000Z | 100.0 Hz, 501 samples
BW.RJOB..EHZ | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
BW.RJOB..EHN | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
BW.RJOB..EHE | 2009-08-24T00:20:28.000000Z - 2009-08-24T00:20:32.790000Z | 100.0 Hz, 480 samples
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 11, 2016

Member

I think this is pretty much ready.. includes trimming all to-be-rotated channel sets to a common time base (see e.g. first three traces; special cases like slightly offset sampling points are not yet taken into account -- we could handle this by resampling/interpolation, but for me this is currently out of scope)

Main question left is whether we leave it as is (as new method) or try to get this in line with the existing rotate method.. opinions?

st = read("/path/to/ffbx_unrotated_gaps.mseed")
inv = read_inventory("/path/to/ffbx.stationxml")
st.rotate_to_zne(inv)

figure_1
figure_2

P.S.: Tests currently fail because I added azimuth/dip to result of Inventory.get_coordinates(). Not sure if we want a new separate get_orientation() method.. (code duplication etc..)?

Member

megies commented Mar 11, 2016

I think this is pretty much ready.. includes trimming all to-be-rotated channel sets to a common time base (see e.g. first three traces; special cases like slightly offset sampling points are not yet taken into account -- we could handle this by resampling/interpolation, but for me this is currently out of scope)

Main question left is whether we leave it as is (as new method) or try to get this in line with the existing rotate method.. opinions?

st = read("/path/to/ffbx_unrotated_gaps.mseed")
inv = read_inventory("/path/to/ffbx.stationxml")
st.rotate_to_zne(inv)

figure_1
figure_2

P.S.: Tests currently fail because I added azimuth/dip to result of Inventory.get_coordinates(). Not sure if we want a new separate get_orientation() method.. (code duplication etc..)?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 14, 2016

Member

I did not yet fully review this but I don't really like the new method name. I see two possibilities:

  • Either name it Stream.rotate_to("ZNE", inventory=inv) - then rotate_to("RT", inventory=inv, event=ev) and similar things can also be made to work.
  • Or just keep the original method name with the function signature we discussed above.

I personally would strongly prefer the second option. We already have a lot of methods on the Stream and Trace object and it will definitely confuse people if we suddenly have two methods to rotate data.

Member

krischer commented Mar 14, 2016

I did not yet fully review this but I don't really like the new method name. I see two possibilities:

  • Either name it Stream.rotate_to("ZNE", inventory=inv) - then rotate_to("RT", inventory=inv, event=ev) and similar things can also be made to work.
  • Or just keep the original method name with the function signature we discussed above.

I personally would strongly prefer the second option. We already have a lot of methods on the Stream and Trace object and it will definitely confuse people if we suddenly have two methods to rotate data.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 14, 2016

Member

but I don't really like the new method name

Yeah, I agree, should ideally be unified with the existing one. Just wanted to get the implementation ready first before worrying about how to plug it in.

Member

megies commented Mar 14, 2016

but I don't really like the new method name

Yeah, I agree, should ideally be unified with the existing one. Just wanted to get the implementation ready first before worrying about how to plug it in.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 14, 2016

Member

Ok, I've plugged the new routine into the existing Stream.rotate() as method="->ZNE".

from obspy import read, read_inventory
st = read("/path/to/ffbx_unrotated_gaps.mseed")
inv = read_inventory("/path/to/ffbx.stationxml")
st.rotate(method="->ZNE", metadata=inv)  # doctest: +ELLIPSIS

This might not be 100% ideal, as some magic inside behaves different from the other methods (cutting to common time base etc.), but.. well..

Member

megies commented Mar 14, 2016

Ok, I've plugged the new routine into the existing Stream.rotate() as method="->ZNE".

from obspy import read, read_inventory
st = read("/path/to/ffbx_unrotated_gaps.mseed")
inv = read_inventory("/path/to/ffbx.stationxml")
st.rotate(method="->ZNE", metadata=inv)  # doctest: +ELLIPSIS

This might not be 100% ideal, as some magic inside behaves different from the other methods (cutting to common time base etc.), but.. well..

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 14, 2016

Member

Interesting syntax with the arrow but I do kind of like it :-)

I would name the metadata kwarg just inventory. That's how we call it in other functions (remove_response(), remove_sensitivity()) - I consider SEED support to be legacy and if the inventory kwarg also has to eat a Parser object - so be it - but I don't think it is worth it to design the syntax around that. It is also a bit ambiguous as a potentially to be implemented ->RT variant will also require event information which is also metadata but another kind of it.

Member

krischer commented Mar 14, 2016

Interesting syntax with the arrow but I do kind of like it :-)

I would name the metadata kwarg just inventory. That's how we call it in other functions (remove_response(), remove_sensitivity()) - I consider SEED support to be legacy and if the inventory kwarg also has to eat a Parser object - so be it - but I don't think it is worth it to design the syntax around that. It is also a bit ambiguous as a potentially to be implemented ->RT variant will also require event information which is also metadata but another kind of it.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 14, 2016

Member

Interesting syntax with the arrow but I do kind of like it :-)

Just going with what was there before.. ;-) I'm not a fan of inventing some special string syntax (like the catalog filter syntax), but here it really is just an enumeration of a handful of valid values, so it should be OK, I think.

I would name the metadata kwarg just inventory. That's how we call it in other functions (remove_response(), remove_sensitivity()) - I consider SEED support to be legacy and if the inventory kwarg also has to eat a Parser object - so be it - but I don't think it is worth it to design the syntax around that. It is also a bit ambiguous as a potentially to be implemented ->RT variant will also require event information which is also metadata but another kind of it.

I'm a bit torn on this one. I think still a lot of people operate on dataless SEED files from past deployments and a naming of inventory will hide it in the detailed documentation (and while this is OK, many people won't bother looking for SEED support in the details if a kwarg's called inventory). For that reason I also named it metadata in PPSD -- because it's able to use an inventory, a seed parser or a filename to a RESP file.

But yes.. a potential clash with metadata for events is a good enough reason for me.

Member

megies commented Mar 14, 2016

Interesting syntax with the arrow but I do kind of like it :-)

Just going with what was there before.. ;-) I'm not a fan of inventing some special string syntax (like the catalog filter syntax), but here it really is just an enumeration of a handful of valid values, so it should be OK, I think.

I would name the metadata kwarg just inventory. That's how we call it in other functions (remove_response(), remove_sensitivity()) - I consider SEED support to be legacy and if the inventory kwarg also has to eat a Parser object - so be it - but I don't think it is worth it to design the syntax around that. It is also a bit ambiguous as a potentially to be implemented ->RT variant will also require event information which is also metadata but another kind of it.

I'm a bit torn on this one. I think still a lot of people operate on dataless SEED files from past deployments and a naming of inventory will hide it in the detailed documentation (and while this is OK, many people won't bother looking for SEED support in the details if a kwarg's called inventory). For that reason I also named it metadata in PPSD -- because it's able to use an inventory, a seed parser or a filename to a RESP file.

But yes.. a potential clash with metadata for events is a good enough reason for me.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 14, 2016

Member

Hmm...all valid points. It could also be called station_meta or something along these lines. Nonetheless: In this case I vote for inventory as its a method on the stream object and other methods on it also use inventory...that would IMHO be a bit confusing if we suddenly have two names. But I can also live with station_meta if you want to go down that route.

Member

krischer commented Mar 14, 2016

Hmm...all valid points. It could also be called station_meta or something along these lines. Nonetheless: In this case I vote for inventory as its a method on the stream object and other methods on it also use inventory...that would IMHO be a bit confusing if we suddenly have two names. But I can also live with station_meta if you want to go down that route.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 14, 2016

Member

Nah, I think I'm convinced.. inventory it is.

Member

megies commented Mar 14, 2016

Nah, I think I'm convinced.. inventory it is.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Aug 30, 2016

Member

We should probably also have ->ZRT and ->LQT to go from arbitrary rotations to these. One application (that we currently don't really cover) are E and N channels that are are not directly pointing towards the east and north but are slightly off. This information is likely part of the inventory object and should thus be used.

Member

krischer commented Aug 30, 2016

We should probably also have ->ZRT and ->LQT to go from arbitrary rotations to these. One application (that we currently don't really cover) are E and N channels that are are not directly pointing towards the east and north but are slightly off. This information is likely part of the inventory object and should thus be used.

@megies megies referenced this pull request Oct 19, 2016

Closed

Release 1.1.0 #1562

19 of 20 tasks complete
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 11, 2017

Member

Rebasing and force-pushing.. quite some conflicts during merging, hope I don't break things.. (old head is at 906f72c for future reference)

Member

megies commented May 11, 2017

Rebasing and force-pushing.. quite some conflicts during merging, hope I don't break things.. (old head is at 906f72c for future reference)

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 11, 2017

Member

Coming back to this one, actually I think this one's ready..

Gotta check the Stream.rotate() docs page when docs buildbot is done..

Member

megies commented May 11, 2017

Coming back to this one, actually I think this one's ready..

Gotta check the Stream.rotate() docs page when docs buildbot is done..

@megies megies changed the title from Make Stream.rotate() take an Inventory object to Make Stream.rotate() take an Inventory object / Stream.rotate from unaligned to ZNE May 11, 2017

@krischer

Looks pretty good to me. A couple minor comments in line.

In the future we could also think about allowing to pass an event objects so the backazimuth (and potentially inclination) could be calculated automatically and we could add a ->RT method.

Show outdated Hide outdated obspy/core/inventory/inventory.py
@@ -332,35 +332,87 @@ def get_response(self, seed_id, datetime):
raise Exception(msg)
return responses[0]
def get_coordinates(self, seed_id, datetime=None):
def _get_channel_metadata(self, seed_id, datetime=None):

This comment has been minimized.

@krischer

krischer May 12, 2017

Member

Any particular reason this is a private method? Probably useful in its own right.

@krischer

krischer May 12, 2017

Member

Any particular reason this is a private method? Probably useful in its own right.

Show outdated Hide outdated obspy/core/inventory/network.py
@@ -275,16 +275,55 @@ def get_coordinates(self, seed_id, datetime=None):
# if channel latitude or longitude is not given use station
data['latitude'] = cha.latitude or sta.latitude
data['longitude'] = cha.longitude or sta.longitude
data['elevation'] = cha.elevation
data['elevation'] = cha.elevation or sta.elevation

This comment has been minimized.

@krischer

krischer May 12, 2017

Member

Minor nitpick as its a bit unlikely in practice - but the three previous lines should be cha.X if cha.X is not None else sta.X.

@krischer

krischer May 12, 2017

Member

Minor nitpick as its a bit unlikely in practice - but the three previous lines should be cha.X if cha.X is not None else sta.X.

Show outdated Hide outdated obspy/core/inventory/network.py
:type seed_id: str
:param seed_id: SEED ID string of channel to get coordinates and
orientation for.
:type datetime: :class:`~obspy.core.utcdatetime.UTCDateTime`, optional

This comment has been minimized.

@krischer

krischer May 12, 2017

Member

The optional here is redundant as its already in the function signature.

@krischer

krischer May 12, 2017

Member

The optional here is redundant as its already in the function signature.

Show outdated Hide outdated obspy/core/inventory/network.py
:type seed_id: str
:param seed_id: SEED ID string of channel to get orientation for.
:type datetime: :class:`~obspy.core.utcdatetime.UTCDateTime`, optional

This comment has been minimized.

@krischer

krischer May 12, 2017

Member

Same thing with optional here.

@krischer

krischer May 12, 2017

Member

Same thing with optional here.

Show outdated Hide outdated obspy/core/stream.py
>>> inv = read_inventory("/path/to/ffbx.stationxml")
>>> st.rotate(method="->ZNE", inventory=inv) # doctest: +ELLIPSIS
<obspy.core.stream.Stream object at 0x...>

This comment has been minimized.

@krischer

krischer May 12, 2017

Member

Would it be better to move this example to the bottom? And maybe add a couple other examples? Otherwise people might not realize that this can also do other kinds of rotations.

@krischer

krischer May 12, 2017

Member

Would it be better to move this example to the bottom? And maybe add a couple other examples? Otherwise people might not realize that this can also do other kinds of rotations.

megies added a commit that referenced this pull request May 12, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer May 12, 2017

Member

Thanks :) Can IMHO be merged if CI passes.

Member

krischer commented May 12, 2017

Thanks :) Can IMHO be merged if CI passes.

megies added a commit that referenced this pull request May 12, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 12, 2017

Member

Fastfail for Travis and Appveyor seems to be working like a charm nowadays..

Force-pushed right now and the old builds were cancelled within seconds, in favor of the new one. :-)

Member

megies commented May 12, 2017

Fastfail for Travis and Appveyor seems to be working like a charm nowadays..

Force-pushed right now and the old builds were cancelled within seconds, in favor of the new one. :-)

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 12, 2017

Member

Fixed a mistake during rebasing.. I'm adding everything in xseed Parser's get_coordinates(), too lazy to also add methods get_orientation and get_metadata as with Inventory.. and it's kind of legacy anyway..

Member

megies commented May 12, 2017

Fixed a mistake during rebasing.. I'm adding everything in xseed Parser's get_coordinates(), too lazy to also add methods get_orientation and get_metadata as with Inventory.. and it's kind of legacy anyway..

megies added a commit that referenced this pull request May 16, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 16, 2017

Member

Python 3 fix + rebased + force-pushed

Member

megies commented May 16, 2017

Python 3 fix + rebased + force-pushed

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 17, 2017

Member

I'm a bit shocked that np.load() with pure array data in npz files is hickuping with UnicodeDecodeError on Python3.. I really thought it's absolutely safe to use..

https://travis-ci.org/obspy/obspy/jobs/232774500#L1032

Member

megies commented May 17, 2017

I'm a bit shocked that np.load() with pure array data in npz files is hickuping with UnicodeDecodeError on Python3.. I really thought it's absolutely safe to use..

https://travis-ci.org/obspy/obspy/jobs/232774500#L1032

megies added a commit that referenced this pull request May 17, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 17, 2017

Member

Tests should pass now..

Member

megies commented May 17, 2017

Tests should pass now..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 17, 2017

Member

Switching test data file to SLIST format..

Member

megies commented May 17, 2017

Switching test data file to SLIST format..

megies added a commit that referenced this pull request May 17, 2017

@megies megies self-assigned this May 18, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 22, 2017

Member

Could be merged after a final review..

Member

megies commented May 22, 2017

Could be merged after a final review..

@trichter

Looks good! I have some minor comments.

Show outdated Hide outdated obspy/core/stream.py
:class:`~obspy.io.xseed.parser.Parser` object) and ignores
``back_azimuth`` and ``inclination`` parameters. Additional
kwargs will be passed on to :meth:`_rotate_to_zne()` (use if
other component pairs than ``["Z", "1", "2"]`` and

This comment has been minimized.

@trichter

trichter Oct 5, 2017

Member

component pairs -> components ? (see other comment)

@trichter

trichter Oct 5, 2017

Member

component pairs -> components ? (see other comment)

This comment has been minimized.

@megies

megies Oct 8, 2017

Member

done

@megies

megies Oct 8, 2017

Member

done

Show outdated Hide outdated obspy/core/stream.py
:class:`~obspy.io.xseed.parser.Parser`
:param inventory: Inventory or Parser with metadata of channels.
:type component_pairs: list or tuple
:param component_pairs: List of combinations of three (case sensitive)

This comment has been minimized.

@trichter

trichter Oct 5, 2017

Member

Why call this parameter 'component_pairs'? Maybe components or original_components fits better?

@trichter

trichter Oct 5, 2017

Member

Why call this parameter 'component_pairs'? Maybe components or original_components fits better?

This comment has been minimized.

@megies

megies Oct 8, 2017

Member

renamed it to components

@megies

megies Oct 8, 2017

Member

renamed it to components

channel=cha_pattern)
st.trim(infos["start"], infos["end"])
for _, _, _, _, start_, end_, _, _ in infos["gaps"]:
st = st.cutout(start_, end_)

This comment has been minimized.

@trichter

trichter Oct 5, 2017

Member

One side effect of the cutout is that it changes the number of traces in the stream if gaps are present, which is a bit unexpected at the first glance. I assume it is no alternative to mask the data?

@trichter

trichter Oct 5, 2017

Member

One side effect of the cutout is that it changes the number of traces in the stream if gaps are present, which is a bit unexpected at the first glance. I assume it is no alternative to mask the data?

This comment has been minimized.

@megies

megies Oct 8, 2017

Member

Yeah, this might be unexpected for users, but in general relying on something like stream[0] in user code without using things like stream.select(component=...) and checking length of that output (which I see really often) is just plain bad practice.

Plus, masking data might lead to even much more obscure problems to unsuspecting users, with all the implications that can come with masked arrays and signal processing operations that work on the underlying numpy arrays. So I think this is by a wide margin the lesser evil.

@megies

megies Oct 8, 2017

Member

Yeah, this might be unexpected for users, but in general relying on something like stream[0] in user code without using things like stream.select(component=...) and checking length of that output (which I see really often) is just plain bad practice.

Plus, masking data might lead to even much more obscure problems to unsuspecting users, with all the implications that can come with masked arrays and signal processing operations that work on the underlying numpy arrays. So I think this is by a wide margin the lesser evil.

# cut data so that we end up with a set of matching pieces for the tree
# components (i.e. cut away any parts where one of the three components
# has no data)
st._trim_common_channels()

This comment has been minimized.

@trichter

trichter Oct 5, 2017

Member

+1 Would be nice to use this method for the other rotations, too

@trichter

trichter Oct 5, 2017

Member

+1 Would be nice to use this method for the other rotations, too

This comment has been minimized.

@megies

megies Oct 8, 2017

Member

Hmm.. to not hold up 1.1.0 any longer, I would propose to handle this in a follow-up issue. Would you mind opening one for 1.2.0?

@megies

megies Oct 8, 2017

Member

Hmm.. to not hold up 1.1.0 any longer, I would propose to handle this in a follow-up issue. Would you mind opening one for 1.2.0?

megies added some commits Mar 4, 2016

add helper routines on Stream to prepare data for 3-C rotation
ensuring all traces (potentially or supposedly) used in rotation have
common time bases (i.e. cut down to timespans for which data on all
channels is present)
add stream method to rotate to ZNE given an inventory
(and specifying sets of three component code letters to combine
refactor new rotation of streams to ZNE based on channel metadata
..(e.g. components "Z", "1" and "2" of borehole instruments) to use common
Stream.rotate() interface.
rename kwarg in `Stream.rotate()` from "metadata" to "inventory"
..to be in line with other Stream methods working with station metadata
(although this particular method also works with Parser objects)
rotate: rename option "component_pairs" to "components"
not in a released version, so not problematic
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 8, 2017

Member

Some last minor adjustments to comments, rebased and force-pushed. Gonna merge this if CI looks good, unless anybody brings up more comments/reviews.

Member

megies commented Oct 8, 2017

Some last minor adjustments to comments, rebased and force-pushed. Gonna merge this if CI looks good, unless anybody brings up more comments/reviews.

@megies megies merged commit 5dbbdd5 into master Oct 8, 2017

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@megies megies deleted the stream_rotate_inv branch Oct 8, 2017

nackerley pushed a commit to nackerley/obspy that referenced this pull request May 8, 2018

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