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

Fixes for Seisan + Nordic files: parsing of int16 waveform files, edge cases in event files, compact event files #3287

Open
wants to merge 22 commits into
base: maintenance_1.4.x
Choose a base branch
from

Conversation

flixha
Copy link
Contributor

@flixha flixha commented Mar 30, 2023

What does this PR do?

  • obspy.io.nordic:
    • Parse ID-lines (type I) into event.extra['nordic_event_id]
    • Support reading of compact Nordic files (only containing header lines)
    • Write amplitude with highest possible precision
    • Fix writing / reading of high-frequency IAMLHF amplitudes
    • Fix parsing of BAZ when they precede the associated phase pick in Nordic file
    • Fix bug where check for nordic format version crashed on long phase names
  • obspy.io.seisan:
    • Fix parsing of files written in int16 (previously read as int32 which resulted in waveforms that looked very similar to the actual waveforms, but had wrong number of samples and wrong amplitudes)

Why was it initiated? Any relevant Issues?

This PR takes care of some additional, rather rare edge cases for reading of Seisan waveform files and Nordic event parameter files. There were not related open issues that this PR resolves.

I realize this PR now combines a few more commits than I originally anticipated, so sorry for that. Let me know if you'd rather have it split up.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds
  • [ ] If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the build_docs tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.
  • [ ] If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the test_network tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • [] Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • [ ] First time contributors have added your name to CONTRIBUTORS.txt .
  • ~[ ] If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts. ~
  • [ ] New modules, add the module to CODEOWNERS with your github handle
  • Add the yellow ready for review label when you are ready for the PR to be reviewed.

for id_line, tag in id_lines:
event_id = id_line.split('ID:')[-1].split(' ')[0].strip('dSLRD')
break
event.extra = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now this might be the only instance setting anything on extra but that might change.. might be safer to do something like

extra = event.setdefault('extra', {})
extra['nordic_event_id'] = ...

try:
header = sorted(tagged_lines['7'], key=lambda tup: tup[1])[0][0]
except IndexError: # Compact catalog file without picks?!
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be OK but indexing is used 4 times on that line, so it might or nmight not be too "catch all" of an exception handling? Up to you though, I don't know the details here too much.

@megies
Copy link
Member

megies commented Mar 31, 2023

I don't know this format at all, so I only have a few comments after briefly looking at this. Otherwise it's looking good to me (again without knowing or using that format).

Two things to consider though:

  • I feel like this is a bit of a mix of things that could go into maintenance branch and some other things that feel like changing too much for that and should rather go into master. It's fine for me to have all this together in this PR but maybe we should rather merge into master then? (can be easily changed up top, only changelog would need moving after a quick rebase on master)
  • Current status is we might still support Python 3.8 for the next set of bugfix+feature release (see discussion in Release 1.5.0 #3228 and raise min versions of numpy, scipy, mpl in setup.py #3087) and then drop it right after, so maybe it might be good to work around that new removeprefix() convenience method

@megies megies added .io issues generally related to our read/write plugins .io.nordic labels Mar 31, 2023
@flixha
Copy link
Contributor Author

flixha commented Apr 5, 2023

Thanks a lot for the review!
I will get to fix your suggestions, may just need a few days until I get on it.
I understand and agree that this may be better suited for merging into master for the next release - after all it fixes relatively rare edge cases while the new functionality would have better gone into a new release anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.io.nordic .io issues generally related to our read/write plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants