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

Refactor filter #49

Merged
merged 10 commits into from
Nov 23, 2024
Merged

Refactor filter #49

merged 10 commits into from
Nov 23, 2024

Conversation

cboulay
Copy link
Contributor

@cboulay cboulay commented Nov 19, 2024

This PR closes #6 by refactoring the filter module with the following changes:

  • new filter_gen_by_design generator method applies the filter designed by the callable passed in as an argument
  • new FilterBase class
    • apply_filter is now gone. Despite not being an underscored method, I think this is safe to change because I don't think this was used anywhere else.
    • abstract method design_filter must return a callable that accepts fs and nothing else. For most filters, this means using a lambda or functools.partial to prepare the typical design function.
  • Filter now inherits this base class and its design_filter is simply a lambda that returns the coefficients from SETTINGS.
  • new ChebyshevFilter with type I or type II designs.
  • new test_filter_system
  • new test_decimate_system because Decimate was affected by the filter change.
  • Modified Decimate to use the new ChebyshevFilter unit instead of the IMO confusing legacy Filter

Importantly, the API is backwards compatible except for the loss of apply_filter and I changed the FilterSettings field name from filt to coefs.

@cboulay
Copy link
Contributor Author

cboulay commented Nov 21, 2024

@griffinmilsap , please let me know if you're OK with the loss of apply_filter. I don't see how this unit method might be used by client code and if you're OK with the field being renamed to coefs from filt. I don't mind changing filt back but I felt like coefs was more consistent with the API.

Copy link
Contributor

@griffinmilsap griffinmilsap left a comment

Choose a reason for hiding this comment

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

@cboulay I decided to take this branch for a run by using it in some of my projects. I ran into a very strange problem upon instantiating ButterworthFilter which resulted in an error when deepcopying the __streams__ property of the class. It turns out that the new FilterCoefsMultiType used in FilterBase.INPUT_FILTER was not deepcopy-able.

This should have resulted in the tests failing, because we instantiate a ButterworthFilter in the tests, but I checked the CI and all tests pass. I ran the tests on my own machine and I saw this same failure. Out of curiosity I used uv to switch the python version to 3.11 and 3.12 and ButterworthFilter worked completely fine in both of those versions and all tests passed, but python 3.10 seems broken on my machine.

Digging deeper, I checked out the CI logs on GitHub Actions and noticed that the CI runner on macos aarch64 is using CPython 3.10.15, but on my local machine (also macos aarch64) I'm getting CPython 3.10.0 after a uv sync --all-extras --dev. I forced a uv python install 3.10.15 and magically everything works and all of my software runs with this branch. I then tracked it down to a bugfix in python 3.10.1 which seems to have resolved the issue. It seems to be related to this issue/fix. python/cpython#89330. This fix got rolled into 3.9.8 and I verified 3.9.7 has the same issue, but we're good with 3.9.10 (uv can't seem to find a version of 3.9.8 or 3.9.9)

So at the end of the day, we're still Python 3.9, 3.10 compatible, but only ^3.9.8 and ^3.10.1. We should probably mark this in the pyproject.toml before merging this branch.

Aside, why was uv only pulling 3.10.0 until just now? After telling it to grab 3.10.15 it seems to default to that now when I ask it to use python 3.10 which is nice. Spose I wouldn't have found this issue if that hadn't been the case...

@cboulay
Copy link
Contributor Author

cboulay commented Nov 22, 2024

Interesting, thanks for tracking that down!
There are a few options I guess.

  1. Forget about the FilterCoefsMultiType class.
  2. Require Python >= 3.10.15; this allows for better annotations all around.
  3. Figure out how to set minimum patch versions for different minor versions.

Option 3 would give the widest compatibility, but unfortunately I can't figure out how to do that. Any ideas?

@griffinmilsap
Copy link
Contributor

griffinmilsap commented Nov 22, 2024

An increasing number of packages are starting to require Python 3.10+. I particularly like the shortening of typing.Union to |. I'm still in favor of keeping core functional down to 3.8 but I've already bumped a few of my packages to minimum 3.10.

Its a little silly though that this whole issue comes up because of the typing on Input/OutputStreams which.. in practice isn't actually checked, used or hinted in any way. I leave it up to you on how to fix. Easiest thing is to probably just toss a typing.Any in to replace FilterCoefsMultiType and we can disregard the whole issue for a little while longer

@cboulay
Copy link
Contributor Author

cboulay commented Nov 22, 2024

@griffinmilsap , please let me know if the modified PR is ready.

Copy link
Contributor

@griffinmilsap griffinmilsap left a comment

Choose a reason for hiding this comment

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

LGTM -- Thanks!!

@cboulay cboulay merged commit e0dec44 into dev Nov 23, 2024
9 checks passed
@cboulay cboulay deleted the 6-filter-refactor branch November 23, 2024 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sigproc.filter -- fs not needed, hangs when there is no design, and other issues
2 participants