Skip to content
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

WIP: Fix bug where perfectly aligning traces are merged into one trace by MSeed read routine #498

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

flixha
Copy link
Collaborator

@flixha flixha commented Jun 20, 2022

What does this PR do?

First attempt at fixing #497. This checks that all template traces have the same length. If it finds any traces that have double (or another multiple) of the "normal" template length, then it splits these traces into traces of the exact same length with the newly corrected start and endtimes.

I will add testing / changes-log once I / we feel that this is the right way to avoid the bug.

Why was it initiated? Any relevant Issues?

The fix here keeps all the checks within EQcorrscan, while in theory I was hoping to find an option that avoids the behavior that leads to this bug in the mseed-routines. But I don't think that such an option exists there for exact matches; the only option I do know is that one case change a hard-coded option in the miniseed-library to reduce the tolerance until which traces are merged. But that only concerns the matching of time samples; not block numbers as far as I know.

As we don't (yet) save template length as a template / tribe metadata, the following edge cases would not be solved by this PR:

  • when all traces of a template are affected by the mseed-segment merging problem
  • maybe other sample

PR Checklist

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

@flixha flixha added the bug label Jun 20, 2022
@calum-chamberlain
Copy link
Member

This is a nice idea to cope with this, but frustratingly the lack of checks for short template lengths in other places can lead to some short traces being included in templates. This means that taking the minimum of the trace lengths to be the correct length probably won't work as a general fix.

I wonder if it is time to add some more details to the template files that are written out, like the nominal length, or the start and end times of each channel in the template.

It would also be good to catch the issue of incorrect template lengths on creation...

@flixha
Copy link
Collaborator Author

flixha commented Jun 21, 2022

I see that; I didn't fully think through that part about templates that are too short because I had my own checks in place while I was creating the templates. So this solution here won't work on its own for now..

@calum-chamberlain
Copy link
Member

Sadly no - I think that generating templates not of the length requested is a bug that should be fixed as well, but that still won't help with older templates, or templates that someone might have constructed not using EQcorrscan (once the bug is fixed). In another issue (I forget which, sorry), allowing different lengths on different channels has been discussed as well. Maybe we should just commit to fixing this by adding more metadata and forcibly reconstructing the waveforms to meet that metadata?

@calum-chamberlain calum-chamberlain added this to the 0.5.0 milestone Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants