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

Energy ratio adjustments #3243

Merged
merged 2 commits into from Dec 20, 2022
Merged

Energy ratio adjustments #3243

merged 2 commits into from Dec 20, 2022

Conversation

megies
Copy link
Member

@megies megies commented Dec 16, 2022

What does this PR do?

Minor adjustments in new energy ratio triggers, raise ValueError on nonsensical input values instead of returning zero-filled arrays.

CC @dizcza

Why was it initiated? Any relevant Issues?

minor adjustement to #3161
see #3161 (review)

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 ready for review tag when you are ready for the PR to be reviewed.

@megies megies added the .signal issues related to our signal processing functionalities label Dec 16, 2022
@megies megies added this to the 1.5.0 milestone Dec 16, 2022
@dizcza
Copy link
Contributor

dizcza commented Dec 16, 2022

LGTM, save for I'd use assertRaises without any expected message as (1) the message could change in the future and (2) you duplicate code by doing that.

Also, consider switching to either pytest or unittest. You have a mix of both and I don't see a reason for that.

@d-chambers
Copy link
Member

Also, consider switching to either pytest or unittest. You have a mix of both and I don't see a reason for that.

The reason the mix exists is because we are transitioning from unittest to pytest. Agreed this is a good time to transition this test file to pytest if @megies is up for it.

@megies
Copy link
Member Author

megies commented Dec 20, 2022

The reason the mix exists is because we are transitioning from unittest to pytest. Agreed this is a good time to transition this test file to pytest if @megies is up for it.

I might do a separate PR for this

@megies megies merged commit 49b8b41 into master Dec 20, 2022
@megies megies deleted the energy_ratio_adjustments branch December 20, 2022 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.signal issues related to our signal processing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants