EQcorrscan icon indicating copy to clipboard operation
EQcorrscan copied to clipboard

More flexible selection of vertical and horizontal channels

Open flixha opened this issue 2 years ago • 4 comments

What does this PR do?

Allows user to choose freely which channels should be associated with P-waves ("vertical channels") and which should be associated with S-waves ("horizontal channels"), and whether picks on one of the vertical channels should be treated to apply to all channels in that group (so like all_horiz).

Why was it initiated? Any relevant Issues?

While EQcorrscan currently has the option to choose parameters all_horiz and to define the horizontal channels with horizontal_chans, there is no equivalent option for the channels primarily associated with P-picks. This can be relevant e.g., when for an OBS deployment, a user wants to have Z- and D-channels (vertical, pressure) associated with the P-wave, and N/E (or 1/2) associated with S-arrivals. For simplicity let's call Z and D the "vertical channels", even though the hydrophone isn't really that.

The options were already in place in some functions, but it wasn't working fully for all relevant functions, i.e., template_gen, lag_calc, and some helper functions.

Remaining points: ~- be consistent with default values for horizontal_chans, which appears both as ['E', 'N', '1', '2', '3'] and ['E', 'N', '1', '2']~ ~- add full test for functionality~

PR Checklist

  • [X] develop base branch selected?
  • [X] This PR is not directly related to an existing issue (which has no PR yet).
  • [X] All tests still pass.
  • [X] Any new features or fixed regressions are be covered via new tests.
  • [X] Any new or changed features have are fully documented.
  • [X] Significant changes have been added to CHANGES.md. ~- [ ] First time contributors have added your name to CONTRIBUTORS.md.~

flixha avatar Jul 21 '22 15:07 flixha

I now added a test case for ocean bottom seismometer data, with 4 channels (HDH and HHZ for P-waves), and with T-arrivals (that should also be handled on the vertical channels).

Tests on python 3.8 and 3.9 are all failing now, I suspect it has to do with a new Scipy version 1.9.0 - looking into that.

flixha avatar Aug 01 '22 12:08 flixha

In Scipy>1.9.0, there is no more "hanning" window; rather it's only accessible now as "hann"; see: https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.windows.hann.html

flixha avatar Aug 01 '22 12:08 flixha

Tests still failing due to the same hanning-issue in obspy (see https://github.com/obspy/obspy/pull/3117), so I'm setting a limit on the Scipy-version (<1.9.0) for now until obspy is updated.

flixha avatar Aug 02 '22 10:08 flixha

Nice, this looks good - do you need anything from me for this? Let me know when you want a review.

calum-chamberlain avatar Aug 02 '22 22:08 calum-chamberlain

Nice, this looks good - do you need anything from me for this? Let me know when you want a review.

I somewhat forgot a bit about the merging of this PR - this is ready for review and if you are happy with it, to be merged into develop!

flixha avatar Dec 12 '22 08:12 flixha

@calum-chamberlain thanks for the review! I see I missed some of the default definitions and they should be the same of course. I was a bit unsure whether '3' should be part of the default or not. When channels are called 1 and 2 in addition to channel Z, then they are assumed to be horizontal. If channels are called 1, 2, and 3, I guess it's not obvious which ones are horizontal or is there some. So better to remove '3' from the default.

This test here contains a trace / picks with channel code '3' in this data file. It only succeeds when horizontal_channels are manually specified to include channel 3. So I adjusted the test, I guess it's useful to keep it like this as an edge case test.

I'm not good at interpreting the coverage tests, but it appears that the change to the test or defaults changed the coverage such that the coverage test complains about indirect coverage changes.. what should I best do about that?

flixha avatar Jan 04 '23 09:01 flixha

Looks good to me, and I don't see the coverage complaints - maybe they were raised before the full test suite was finished? Looks good to me so I'm going to merge. Thanks for adding that edge case test.

As an aside, that log handler looks like it is probably handy. If you found that helpful in debugging tests you might consider adding a wrapper that can be added to all tests to set up logging for them.

calum-chamberlain avatar Jan 04 '23 20:01 calum-chamberlain