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

Allow to specify filter freqs as args, Add different filter types #3294

Merged
merged 4 commits into from May 16, 2024

Conversation

trichter
Copy link
Member

@trichter trichter commented Apr 20, 2023

What does this PR do?

Here I suggest two enhancements to ObsPy's filter functions:

  1. Allow to specify filter frequencies as arguments, e.g. stream.filter('lowpass', 1) can be used as a replacement for stream.filter('lowpass', freq=1)
  2. Allow to use all filter types implemented in scipy's iirfilter design function, i.e. 'butter', 'cheby1', 'cheby2', 'ellip', 'bessel', not only the default Butterwoth filter.

Documentation still missing; first I want to get some feedback.

Why was it initiated? Any relevant Issues?

Enhancement.

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.

@trichter trichter added .core issues affecting our functionality at the very core .signal issues related to our signal processing functionalities labels Apr 20, 2023
@trichter trichter changed the base branch from master to filter_matrix April 20, 2023 10:53
@d-chambers
Copy link
Member

first I want to get some feedback.

I like the idea of allowing positional parameters once the filter type is specified, and the increased flexibility could be nice, provided backwards compatibility is maintained.

One comment on the API though; I have always found the filter method a bit complicated to use. The required keyword arguments change based on the filter type, making the documentation and usage a bit confusing. Why not have separate stream/trace methods for each filter type defined in obspy.signal.filter? I guess it is so that filter can dispatch to any functions defined with the filter entry point but I haven't ever seen that used by outside code. Of course, we can't make any changes to filter now, but perhaps just adding new, better named filter methods would be a fruitful route to improve usability?

@trichter
Copy link
Member Author

trichter commented Apr 26, 2023

Of course, we can't make any changes to filter now, but perhaps just adding new, better named filter methods would be a fruitful route to improve usability?

To be honest, I like that there is only a single filter method. I see your point with the different kwargs depending on the plugin used. Similar to the read function one needs to read the documentation of the functions linked in the doc of Trace.filter. I guess introducing more filter functions would lead to more confusion.

Edit: Another possible simplification of the code could be to dispatch the four filter functions to a single function in the signal module. That would de-duplicate docs and some code.

Base automatically changed from filter_matrix to master April 26, 2023 11:45
@megies
Copy link
Member

megies commented May 26, 2023

Sorry about the late reply..

Re: more methods

I kind of agree with Tom that it might be good to keep the "one filter" method we have right now. Mainly because it it very clear for people where to look for filtering and have all documentation in one spot and you can see a nice overview of all the available filtering methods. And secondly because Stream/Trace already have a massively cluttered list of attached methods, so I think some funneling/grouping is very good to have and I'd probably vote for extending on the current way we do it with the type first arg to .filter().
If we're bold we could just try to import and use any not defined by us type passed in by the user from scipy.signal and use it. (forget about that part, just looked at the details and saw how that ftype thing is used for that switch)

Re: usage with positional args

Hmm.. I can see where you are coming from, it is slightly annoying to have to look up what the expected kwargs are named sometimes. On the other hand it's much safer and it was my impression that python is kind of steering a bit into the direction of preventing excessive/unsafe args usage with the invention of keyword-only arguments.
But on the other hand, treating users as grown ups and let them decide if they wanna more concise or "safer" also has a valid point. So I guess I'm kind of undecided and if you want to make this change I don't really strongly oppose it. Just looking at the code changes though, it kinda feels like it might get confusing how args/kwargs get passed through several internal methods to arrive at the finally called filter method..

obspy/core/stream.py Outdated Show resolved Hide resolved
@megies megies added this to the 1.5.0 milestone May 26, 2023
@trichter trichter added ready for review PRs that are ready to be reviewed to get marked ready to merge build_docs Docs will be automatically built and deployed in github actions on pushes to the PR labels Jan 30, 2024
@trichter
Copy link
Member Author

Preview of changed docs is here, here, here, also see the other filter functions in obspy.signal.

@megies
Copy link
Member

megies commented Feb 29, 2024

Rebased to resolve conflicts

@megies megies merged commit 4110e43 into master May 16, 2024
29 checks passed
@megies megies deleted the filter branch May 16, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_docs Docs will be automatically built and deployed in github actions on pushes to the PR .core issues affecting our functionality at the very core ready for review PRs that are ready to be reviewed to get marked ready to merge .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