Add harmonic decomposition to rf #48
Conversation
|
Hi, Yes, nice of you to add some docs. A tutorial is a big plus. And we will need a test (which could be similar code as in the tutorial or you can reuse other existing code in a stripped down version). I just pushed some commits to get the tests working again. This branch needs a rebase on master. I will add some inline comments and will do a more thorough review later. |
rf/harmonics.py
Outdated
| from rf.util import _add_processing_info | ||
|
|
||
| @_add_processing_info | ||
| def harmonics(stream, first_comp='R',second_comp=None, azim=0, method='time', **kwargs): |
There was a problem hiding this comment.
You could use a single argument components='R' or similar instead of the two arguments for fist/second component. The logic of the function could be the same.
rf/imaging.py
Outdated
| if show_traces: | ||
| if scale > 0: # scale to all-trace max | ||
| _plot(ax1, times, tr.data / max_ * scale, i + 1) | ||
| elif scale < 0: # scale trace by trace |
There was a problem hiding this comment.
Can you please use a new kwarg for this functionality, a negative scale value is a quick fix, but not intuitive to others.
rf/imaging.py
Outdated
| @@ -139,8 +154,10 @@ def _plot(ax, t, d, i): | |||
| warnings.warn('Different stations or channels in one RF plot. ' + | |||
| 'Do not plot stack.') | |||
| elif len(stack) == 1: | |||
| ax2 = fig.add_axes([fl, 1 - ft - hs, fw2, hs], sharex=ax1) | |||
| if show_traces: ax2 = fig.add_axes([fl, 1 - ft - hs, fw2, hs], sharex=ax1) | |||
There was a problem hiding this comment.
Pleas start a new line after if ... :
rf/imaging.py
Outdated
| if hd2: | ||
| hd2 = hd2.slice2(*trim, reftime='onset') | ||
| except AttributeError: # if it's obspy.Stream() instead of RFStream() | ||
| pass # we might still be ok |
There was a problem hiding this comment.
better raise a meaningful error or warning
|
Thanks, Tom! I did a rebase and addressed those first few comments you gave. I'll work on docs/tests and an example soon. |
|
Ok, I added some documentation (including a tutorial notebook with simple synthetics) and tests - it seems to have broken something in sphinx but I'm not sure what. Let me know what you think. |
|
The broken documentation is a feature of the master branch. I will solve it there. |
|
The documentation build is working again. Also, I was curious why the tests were not running on your branch, I hope this is fixed now. Can you please rebase onto the master branch again? |
|
Thanks, Tom. I made a bit of a mess with the commit history but it looks like the docs are ok now. I think the rest of the tests are going to keep failing here because of the mtspec requirements which I hopefully fixed in a separate PR? Not sure why they pass on master since conda is definitely not installing mtspec for 3.10 and 3.11. |
…t is not available
|
I fixed the commit history and made some style changes. Looks good to me! Nice documentation too. Thanks! The PR is ready to be merged from my side, if you don't want to add anything. |
|
Thanks - all looks good to me! |
|
Thanks again. I will prepare a new release soonish. |
I've added functions for calculating harmonics and for plotting them. So far I haven't made any documentation updates - if this is something you're ok with adding and it looks like it's in decent shape, I'll work on docs and maybe a notebook example.
There's also a commit or two from a small plotting change I made a while back to allow plotting a stacked trace on its own with imaging.plot_rf(); apologies for mixing that in here, I just realized that it ended up on this branch.