Firwin macro #85
Firwin macro #85
Conversation
To implement firwin, there is a need to get_windows on all window types defined in the Scipy signal.windows namespace, most of which are implemented in a _windows.py file. This commit keeps all the window variants that needs to be implemented as comments in an Enum for future implementation, along with some functions that are used across the implementation of all window types. The intended strategy to combat the variadic function type associated to the Scipy's get_windows function is to enclose all function arguments in a struct that represents the given function type, while ensuring all structs representing the Windows type implement a common trait.
The error variants here should guide the user as to the wrong use of functions, instead of taking down the entire program with a panic, which is especially necessary in an embedded environment.
We also introduce the convenience enum to go along with the function.
There still is a need for double specification of window size across both firwin and the Window struct that uses it, unless we have a string to enum converter I suppose...; Either way, its suboptimal currently.
Older commits didn't utilise some properities afforded by Real and RealField. Also added a useless(?) cfg(attr = "alloc") compiler attribute for consistency.
Scipy returns a value error in this case.
Scoped behind standard as a doc test to demonstrate how to utilise the macro. Mandatory imports should be removed in the future as the macro is made more explicit.
This is to make sure the macro scoping works.
Wrong argument was being signalled in the message.
Currently, it is still ordered really messily... Does not handle proper imports... Also does not scope into the appropriate namespace due to the way macros just are....
Utilise `#[doc(inline)]` and `#[doc(never)]` to hide name from crate root and show up in `::signal` namespace instead.
Scope the import within the macro.
SpookyYomo
force-pushed
the
firwin_macro
branch
from
f17b370 to
1615277
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 14
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| let (m, needs_trunc) = extend(self.m, self.sym); | ||
| let n = (0..m); | ||
| let alpha = W::from(self.m - 1).unwrap() / W::from(2).unwrap(); |
There was a problem hiding this comment.
Kaiser window alpha uses wrong size for periodic windows
The alpha calculation uses self.m instead of the extended m when computing the Kaiser window. When sym=false, the extend function sets m = self.m + 1, but alpha is incorrectly computed as (self.m - 1) / 2 instead of (m - 1) / 2. This produces incorrect window values for periodic windows (used in spectral analysis). Other window implementations like general_cosine and general_gaussian correctly use the extended m value. The existing tests only cover sym=true, so this bug would not be caught.
| impl fmt::Display for Error { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Error Display implementation panics with todo!()
The Display trait implementation for Error contains a todo!() macro that will panic at runtime if the error is ever formatted. This occurs when using the {} format specifier, calling .to_string(), or using the ? operator in functions returning String errors. Since firwin_dyn and other functions return Result<_, Error>, any code that attempts to display these errors will crash.
| #[inline(always)] | ||
| fn len_guard(m: usize) -> bool { | ||
| m <= 1 | ||
| } |
There was a problem hiding this comment.
Window functions return empty vector for m=1 instead of [1.0]
The len_guard function returns true for m <= 1, and all window implementations return an empty vector when this happens. However, scipy returns np.ones(M) in this case, which means for m=1 it returns [1.0], not an empty array. All window implementations (Blackman, Boxcar, GeneralCosine, GeneralGaussian, Kaiser, Triangle, Hamming, Nuttall) are affected by this pattern, causing incorrect behavior when m=1.
Additional Locations (2)
| ( ("kaiser", $beta:expr), $m:expr, $sym:expr ) => {{ | ||
| use $crate::signal::windows::GetWindow; | ||
| $crate::signal::windows::Kaiser::new($m, $beta, !$sym).get_window() | ||
| }}; |
There was a problem hiding this comment.
Kaiser macro uses inverted symmetry parameter unlike other windows
The Kaiser macro uses !$sym when constructing the window, while all other window macros (hamming, blackman, triangle, etc.) use $sym directly. This inconsistency means get_window!(("kaiser", 14), 51, true) produces a periodic window (sym=false), while get_window!("hamming", 51, true) produces a symmetric window (sym=true). The Kaiser macro should use $sym directly for consistency with other windows.
1 participant
If the way to call FIR windows is too complicated, macros to have python-esque
functionality is also a viable means.
Blockers:
Note
Implements firwin-based FIR filter design with Kaiser helpers, introduces a comprehensive windows API (traits, builders, macro), and wires it into signal design; adds error type and tests.
firwin_dyn: Add FIR window-method design insignal/filter/design/firwin.rswith input validation, window application, scaling, and extensive tests.kaiser_beta,kaiser_atten,kaiserordindesign/kaiser.rswith tests; re-export indesign/mod.rsand expose viasignal::filter::design.signal/windowsmodule:GetWindowtrait,Windowenum,GetWindowBuilder, and factoryget_window.Boxcar,Triangle,Blackman,Hamming,Nuttall,Kaiser,GeneralCosine,GeneralGaussian,GeneralHamming, each with tests.signal::get_windowfor Python-like usage.error::Errorenum used by new design APIs; export inlib.rs.signalandfiltermodule docs/exports to surface new design and windows functionality; minor wave docs cleanup.Written by Cursor Bugbot for commit 94a951f. This will update automatically on new commits. Configure here.