search

trichter / rf

Receiver function calculation in seismology
MIT License
106 stars 62 forks source link

Add harmonic decomposition to rf #48

Closed hfmark closed 4 months ago

hfmark commented 5 months ago

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.

trichter commented 5 months ago

Hi, Thanks for contributing more code! Of course, I am OK with adding more of your code. I did not work on harmonic decomposition on my own, though.

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.

hfmark commented 5 months ago

Thanks, Tom! I did a rebase and addressed those first few comments you gave. I'll work on docs/tests and an example soon.

hfmark commented 4 months ago

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.

trichter commented 4 months ago

The broken documentation is a feature of the master branch. I will solve it there.

trichter commented 4 months ago

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?

hfmark commented 4 months ago

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.

trichter commented 4 months ago

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.

hfmark commented 4 months ago

Thanks - all looks good to me!

trichter commented 4 months ago

Thanks again. I will prepare a new release soonish.