Merged
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.
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.
This however does not specify to the end user which arguments are raising the error. Fix "Conflict" typo in Error enum variant
ndarray_conv provides linear and FFT convolution for N-dimensional tensors. This commit thus introduces into core under a "num_rs" namespace, since there are many scipy functions that use numpy functions. The ConvolveMode enum is thus moved, and the relevant enum variants of ndarray_conv is also provisioned by means of `.into()`.
Display trait is for when `main` ends and an error message is shown to the end user.
There will be other functions in sci-rs::signal space that uses the linear convolution.
One can use into the Make and ArrayView from Array but not the other way round, this makes the function signature abit more friendly without needing to use `.to_owned()` needlessly.
There is nothing distinguishing between kernel and signal with both arguments now being identical in type.
This is to ensure both branches are tracking Cargo.toml in sci-rs-core
Also add tests for lfilter. Note that `lanes_mut` etc are parrallelizeable with Rayon, and may need more work in the future.
This was caught by clippy.
Suggested by clippy on the website. It is yet to be apparent if this provisioned type necessarily eases the reasoning of the return of the functions.
This avoids the use of RawData and makes return type T more explicit with container S.
The shape as `[usize; N]` function is not used by just lfilter. Also changed the name and documentation to reflect the `const N` requirement.
Assume init for uninit arrays instead of specifying zeros with required shape gives an improved speed in lfilter.
Tried to see in Cargo-asm if this is inlined but `cargo-show-asm` isn't picking up `lfilter` as a function. This will have to be tested in the benching function.
This centralizes areas of concern.
Callee should have the responsibility of constructing the `Axis` object.
These are fairly generic functions that parse user input into valid values of usizes (in their respective context being the arrays they act on), and thus are more suited to be in array tools.
Some of the trait requirements bound to lfilter as a function can be removed.
It appears that regardless for sizes 50 and 500, scipy takes 95-101ms whilst our implementation scales with decimation factor, with decimation factor of 50 taking 31ms and 500 taking 340ms?
`feature = "test"` was unintentionally left out in the previous commit.
![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 December 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.
SpookyYomo
force-pushed
the
lfilter_bench
branch
from
5c603f4 to
bb617fc
Compare
Wrong argument was being signalled in the message.
This is needed for generating Finite Impulse Response windows that will be used by lfilter in benching.
SpookyYomo
force-pushed
the
lfilter_bench
branch
7 times, most recently
from
726c36e to
4cb6cf9
Compare
SpookyYomo
force-pushed
the
lfilter_bench
branch
from
4cb6cf9 to
5b112b3
Compare
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
2 participants
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.
Adds a benchmark for lfilter with 50 element wide FIR window.
Blockers:
randto 0.9.2 #82Note
Introduces sci-rs-core with ndarray-based convolution and errors, adds FIR design (firwin/kaiser) and comprehensive window utilities, implements ndarray lfilter/lfilter_zi, integrates across sci-rs, and adds an lfilter benchmark.
sci-rs-core):Errortype and ndarray-basednum_rs::convolve(Full/Same/Valid) usingndarray_conv.firwin_dynwith validation, scaling, and window selection; add Kaiser helpers (kaiser_beta,kaiser_atten,kaiserord).windowsmodule:get_windowAPI and window types (e.g.,Boxcar,Triangle,Blackman,Hamming,Nuttall,Kaiser,General*).lfiltertrait/function (with optionalzi) andlfilter_zi.ConvolveModefrom core; update FFT-based convolve/correlate tests and rand usage.benches/lfilter.rsCriterion benchmark generating synthetic signals and applying FIRlfilter.sci-rs-coreas dependency and feature-gated (alloc/std); updaterandto 0.9; extendsignalmodule docs/exports.Written by Cursor Bugbot for commit 5b112b3. This will update automatically on new commits. Configure here.