seismopy / mopy

A python package to calculate seismic source parameters
11 stars 1 forks source link

Create set_ methods for `ChannelInfo` #2

Open sboltz opened 5 years ago

sboltz commented 5 years ago

Create the following convenience methods for adding stuff to a ChannelInfo for the basic interface:

sboltz commented 5 years ago

In thinking more about this, unless we decide it's important later, I'm not going to extract the arrival information at this point. There's only one thing that I think I would pull from the arrival object, which is the takeoff_angle, and I don't think that identifying the correct set of arrivals and parsing them is worth the effort.

sboltz commented 5 years ago

@d-chambers, @sgjholt I have a question about the behavior of the set_velocity, set_density, etc., methods.

Currently when a ChannelInfo is instantiated and picks are added, the other parameters get populated using default values, or through .mopy.py. While it should automatically compute the distance and azimuth and things that shouldn't change at this point, should we actually wait to populate the rest with default values until it gets passed to something else (i.e., a TraceGroup), at which point it would issue a warning, or until the user maybe calls an update_parameters or update_meta method?

My thought is this would make it a little more transparent to the user what is happening, and also reminds them that they haven't passed anything to these parameters yet.

Thoughts?

sgjholt commented 5 years ago

@sboltz @d-chambers I think this is a great idea. I think we should have it fall back on the defaults, but issue a warning 1. if they are using defaults and 2. if they try to update without changing from the defaults. What do you think?

d-chambers commented 5 years ago

@sboltz, yes, I agree. I think we should try to separate the mopy-specific bits from the generally useful TraceGroup and StatsGroup (which I think would fit nicely into ObsPlus at some point), so having a method, or a function, which adds the mopy specific rows is a great idea.

However, I don't think we need to wait until the TraceGroup gets the StatsGroup object to call it (although we could).

@sgjholt I am not sure what you mean about try to update in this context, do you mean try to calculate source parameters?

sboltz commented 5 years ago

However, I don't think we need to wait until the TraceGroup gets the StatsGroup object to call it (although we could).

Right. I guess what I meant is if an object that is trying to use the StatsGroup needs those parameters and they haven't been populated yet, then that would get called as a fallback (not quite sure if there is a way to do that on the StatsGroup directly, or if the thing trying to use it would have to call it). Or, if the user wanted to call it directly they could.

I think we should try to separate the mopy-specific bits from the generally useful TraceGroup and StatsGroup (which I think would fit nicely into ObsPlus at some point), so having a method, or a function, which adds the mopy specific rows is a great idea.

So I don't end up redoing a lot of work, what are you considering mopy-specific bits, and how would you recommend separating them for purposes the high-level interface to StatsGroup? Would the set_xyz methods be better served for the MagCalculator class or whatever is probably going to end up wrapping the StatsGroup?

d-chambers commented 5 years ago

@sboltz I think we will end up subclassing or wrapping StatsGroup so pycharm should take care of the refactoring for us when we get to that point. We will just need to decide which methods belong in the general case and which in the mopy case. The set_xyz family should still be StatsGroup methods.

sboltz commented 5 years ago

@d-chambers Sounds good.

sgjholt commented 5 years ago

@d-chambers I was tired when making that comment, so it makes no sense. It should just issue a warning where you're trying to fit spectra using default parameters.

sboltz commented 4 years ago

Note to self: the way that I set the relative time windows (and possibly the absolute time windows?) has a potential for overwriting existing values. This needs to get fixed sooner rather than later, but I don't want to do it right this moment.