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

SAC: Fix sampling rate floating point accuracy edge cases #3446

Merged
merged 7 commits into from
May 17, 2024

Conversation

megies
Copy link
Member

@megies megies commented May 15, 2024

What does this PR do?

Fixes reading some SAC files that suffer from floating point accuracy issues with their sampling interval stored in single precision.

Makes it opt-out, so the rounding can be deactivated and also it will show a warning if the rounding actually changed things during reading SAC.

Why was it initiated? Any relevant Issues?

fixes #3408

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.

otherwise sampling rate can suffer from floating point accuracy issues
due to the single precision used in SAC to store sample spacing in the
trace header
@megies megies added the .io.sac label May 15, 2024
@megies megies added this to the 1.5.0 milestone May 15, 2024
@megies megies requested a review from jkmacc-LANL as a code owner May 15, 2024 10:55
@megies megies added the ready for review PRs that are ready to be reviewed to get marked ready to merge label May 15, 2024
@megies megies changed the title Fix sac sampling rate SAC: Fix sampling rate floating point accuracy edge cases May 15, 2024
Copy link
Contributor

@jkmacc-LANL jkmacc-LANL left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! All these changes just to round a float 😄

@megies
Copy link
Member Author

megies commented May 17, 2024

Looks good, thank you! All these changes just to round a float 😄

🤣 It's like always, making the change would be a one-liner and about one minute of work, but thinking it through, documenting it properly, letting people know when it happens by showing a warning and properly testing both the actual change and the warning as well is 99% of the work and makes it a long task. That's how you see all these software projects from publications that are unusable to other people but fast to get a citation 😜

I do believe it's one of our strong points though, to be robust, reliable and transparent. Even if it slows us down these days (well more people dedicating time would help too 🤣).

@megies megies merged commit 81cdb84 into master May 17, 2024
30 checks passed
@megies megies deleted the fix_sac_sampling_rate branch May 17, 2024 06:26
@jkmacc-LANL
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.io.sac ready for review PRs that are ready to be reviewed to get marked ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SAC: Potential problem with floating point accuracy in sampling rate / sample spacing
2 participants