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

attach_coordinates() method for Trace and Stream #1842

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@claudiodsf
Member

claudiodsf commented Jul 13, 2017

Similar to attach_response(), this method search for and attaches coordinates from an Inventory object.

@claudiodsf

This comment has been minimized.

Show comment
Hide comment
@claudiodsf

claudiodsf Jul 13, 2017

Member

Is doctests enough? Or do you want explicit testing?

Member

claudiodsf commented Jul 13, 2017

Is doctests enough? Or do you want explicit testing?

@claudiodsf claudiodsf requested a review from krischer Jul 13, 2017

attach_coordinates() method for Trace and Stream
Similar to attach_response(), this method search for and attaches
coordinates from an Inventory object.
@claudiodsf

This comment has been minimized.

Show comment
Hide comment
@claudiodsf

claudiodsf Jul 13, 2017

Member

+DOCS please

Member

claudiodsf commented Jul 13, 2017

+DOCS please

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jul 13, 2017

Member

Actually, @krischer and me too, we've kind of arrived at the opinion that inventing attach_response() was a design mistake. At a first glance it seems super convenient, but when you look at it in detail it..

  • adds non-transparent magic, by shifting weight to essentially undocumented (mis-?)use of non-default keys in Trace.stats
  • adds problematic scenarios, mostly stemming from the above non-default-ness of additional keys in Trace.stats that are not handled canonically when making transformations on traces (e.g. what happens when merging two traces with different responses/coordinates?)

I think I'm the one to blame for adding attach_response(), not putting enough thought, following along with the old use of Trace.stats.paz that we used sloppily already from very old times. But.. we already did some (only initially) slightly painful backtracking by introducing inventory as first arg in Trace.remove_response() to get rid of Trace.stats misuse, and I fear this PR will not get a nod, I hope the above was enough to convince you.

As I said, we made the same mistake several times but recognized now that this is not the right way to go. Wherever you need coordinates, you should put inventory as first arg to avoid non-transparent stats magic. And finally.. we discussed this many times, at the end of the day what we want here is a canonical new object structure, working title "DataSet", that combines waveform+station[+event] data.

Edit: The _get_coordinates() helper routine you're proposing for Trace is a valuable piece that could be useful even if we don't agree on the attach_coordinates.

Edit: maybe we should deprecate attach_response() to make clear we discourage non-default keys in stats?

Member

megies commented Jul 13, 2017

Actually, @krischer and me too, we've kind of arrived at the opinion that inventing attach_response() was a design mistake. At a first glance it seems super convenient, but when you look at it in detail it..

  • adds non-transparent magic, by shifting weight to essentially undocumented (mis-?)use of non-default keys in Trace.stats
  • adds problematic scenarios, mostly stemming from the above non-default-ness of additional keys in Trace.stats that are not handled canonically when making transformations on traces (e.g. what happens when merging two traces with different responses/coordinates?)

I think I'm the one to blame for adding attach_response(), not putting enough thought, following along with the old use of Trace.stats.paz that we used sloppily already from very old times. But.. we already did some (only initially) slightly painful backtracking by introducing inventory as first arg in Trace.remove_response() to get rid of Trace.stats misuse, and I fear this PR will not get a nod, I hope the above was enough to convince you.

As I said, we made the same mistake several times but recognized now that this is not the right way to go. Wherever you need coordinates, you should put inventory as first arg to avoid non-transparent stats magic. And finally.. we discussed this many times, at the end of the day what we want here is a canonical new object structure, working title "DataSet", that combines waveform+station[+event] data.

Edit: The _get_coordinates() helper routine you're proposing for Trace is a valuable piece that could be useful even if we don't agree on the attach_coordinates.

Edit: maybe we should deprecate attach_response() to make clear we discourage non-default keys in stats?

@claudiodsf

This comment has been minimized.

Show comment
Hide comment
@claudiodsf

claudiodsf Jul 13, 2017

Member

@megies I'm fine with that. Just pay attention to the fact that in some cases trace.stats.coordinates seems required, like for instance in Stream.plot() (https://docs.obspy.org/packages/autogen/obspy.core.stream.Stream.plot.html)

Member

claudiodsf commented Jul 13, 2017

@megies I'm fine with that. Just pay attention to the fact that in some cases trace.stats.coordinates seems required, like for instance in Stream.plot() (https://docs.obspy.org/packages/autogen/obspy.core.stream.Stream.plot.html)

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jul 14, 2017

Member

Just pay attention to the fact that in some cases trace.stats.coordinates seems required, like for instance in Stream.plot()

...or in https://docs.obspy.org/packages/autogen/obspy.signal.array_analysis.get_geometry.html

Yes, I know. And all of these instances have to be considered bad legacy practice.. :-/

Member

megies commented Jul 14, 2017

Just pay attention to the fact that in some cases trace.stats.coordinates seems required, like for instance in Stream.plot()

...or in https://docs.obspy.org/packages/autogen/obspy.signal.array_analysis.get_geometry.html

Yes, I know. And all of these instances have to be considered bad legacy practice.. :-/

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Aug 7, 2017

Member

I second @megies view. All these instances are design mistakes in retrospect and we should rather add inventory arguments to all of these to augment them.

I'm also not sure if the _get_coordinates() is needed as inv.get_coordinates(tr.id) is IMHO pretty simple to use.

Member

krischer commented Aug 7, 2017

I second @megies view. All these instances are design mistakes in retrospect and we should rather add inventory arguments to all of these to augment them.

I'm also not sure if the _get_coordinates() is needed as inv.get_coordinates(tr.id) is IMHO pretty simple to use.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Aug 8, 2017

Member

@claudiodsf if you want to put some effort into this, you could channel your energy towards the long planned DataSet object. It's supposed to eventually circumvent all these dirty helper stuff, like attaching response to traces and so on..

Member

megies commented Aug 8, 2017

@claudiodsf if you want to put some effort into this, you could channel your energy towards the long planned DataSet object. It's supposed to eventually circumvent all these dirty helper stuff, like attaching response to traces and so on..

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