feat: implement firwin#29
Merged
Merged
Conversation
thomaspmurphy
commented
Mar 8, 2026
polvalente
reviewed
Mar 8, 2026
polvalente
left a comment
Collaborator
There was a problem hiding this comment.
Most comments I left are readability-oriented! Great work :)
After we converge on this and merge the PR we can release a new version
Collaborator
|
I forgot to comment: you can also modify the guides given the PR description. I don't have a hard preference on whether to do it in this PR or in a follow-up after merging. Just let me know which it'll be so that we can release a new version with the updated guides included :) |
Contributor
Author
|
Very helpful feedback, thank you. Of course, I'll update the Livebook in this branch, I was just uncertain about the chicken-and-egg of bumping the version in the notebook (that can be done with the release I guess!). |
thomaspmurphy
commented
Mar 9, 2026
polvalente
approved these changes
Mar 9, 2026
polvalente
approved these changes
Mar 9, 2026
Collaborator
|
@thomaspmurphy can you run the formatter please? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements
NxSignal.Filters.firwin/3, which makes FIR filter design possible using nx_signal's existing resources. Given a tap count, one or more cutoff frequencies, and a window name, it returns a vector of FIR filter coefficients that can then be applied to a signal withNxSignal.Convolution.convolve/3.firwinlives inNxSignal.Filtersas adeftransform. Because the number of cutoff frequencies is a compile-time constant (a literal list), the passband enumeration (Enum.chunk_every,Enum.filter,Enum.reduce) runs at JIT-compile time and emits a statically-unrolled Nx expression graph.Reference coefficients for testing were generated with$I_0$ rather than the Chebyshev series used by NumPy/SciPy, which introduces slightly larger errors for small arguments. This is a known divergence introduced in #27.
scipy.signal.firwin(SciPy 1.x, float64) and hard-coded in the test file. Tests useassert_all_closewithatol: 1.0e-5to account for the f32 vs f64 arithmetic difference. The Kaiser window tests use a looseratol: 1.0e-3becauseNxSignal.Windows.kaiseruses a polynomial/asymptotic Bessel function approximation forPossible follow-up:
guides/filtering.livemdI noticed that there are a couple of bugs in the existing filtering Livebook due to structural changes. Also, the introduction of this filter design method could simplify some of the manual setup in that file if accepted.
Line 61 of the current guide calls
NxSignal.Filters.sinc/1, which does not exist. The sinc function now lives inNxSignal.Waveforms.Line 63 calls
NxSignal.Windows.hann(n: window_length). After the window API was refactored to take a mandatory positional integer argument, this passes a keyword list as the first argument, which fails thewhen is_integer(n)guard:The correct call is
NxSignal.Windows.hann(window_length)I think."FFT - 1200 Hz Low-Pass filter (Hann window); L = 1764". However, the filter in the notebook is 600 Hz withwindow_length = 2048taps. This is probably a copy-paste artefact from an earlier version of the guide.In terms of changes introduced here, the entire "Preparing the Filter" section currently does manually what
firwinnow encapsulates:So it could be simplified thus:
The
windowvariable in the guide is currently serving double duty: it is used for both the FIR filter design and as the STFT analysis window passed toNxSignal.stft/3andNxSignal.istft/3. After the simplification, these two concerns would be separated:stft_windowwould then replace every occurrence ofwindowin thestftandistftcalls. scipy also has separate concepts for a filter design window (symmetric, used once to shape the FIR coefficients) and an analysis window (periodic, used repeatedly in STFT frames) -firwindefaults to symmetric andstfttowindow='hann_periodic'.It would perhaps also be appropriate to add a short "Direct convolution" subsection before the existing STFT-based filtering section, demonstrating the simplest end-to-end usage of
firwin:Proposed changes (this is all working nicely locally using my local version of nx_signal in the deps)
I would be happy to add a PR addressing these. I am quite new to Elixir apologies if my code is not idiomatic—happy to work to improve it!