lfilter #78
Open
lfilter #78
Conversation
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
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
Contributor
Author
|
The complexity has to be increased for the linter to not complain (hence the CI marking this as an issue), or divergence from how the scipy function returns a tuple if |
Unfortunately, it's not quite possible to specify at compile time that `axis: isize` sastisifes the bounds wrt `N`. Also check that a[0] exists and is non-zero.
Variable lifetime capture through reborrow only for subsequent `assign_to` is unnecessary.
This is later checked in `_validate_x()` function. We instead frontload it into the body.
This version of ndarray-conv accounts for both cross-correlation and convolution.
This updates ndarray-conv to 0.5.0.
Behavior of convolve in ndarray-conv was changed with 0.5.0.
This is a squash of 3 commits: 1. Failing unit test with a panic atm, more work needs to be done. 2. Further lfilter fix 3. Fix lfilter for 1D signal Had to step into python to actually see what the slicing was doing...
`alloc` feature flag for lfilter is already specified in the parent module.
Progressively make the test cases more complex so that its easier to identify. We also clarify the filter as being FIR as the `a` polynomial has been set to 1.
This helps reduce from the flow of lfilter code.
Found within .cc file. Only provide the function signature first. This function is called when `a.len() > 1`. Function signature of lfilter is also changed to reflect what zi is expected to be from both lfilter and linear_filter.
Made it clear that it was taking the shape instead of the ndim of a NDArray and making into a array `[T; N]`.
Instead of refactoring the non-zi code to follow the structure of zi code, which results in an additional heap allocation, we instead branch the code so that the scenario where zi is None does not need to undergo the additional heap allocation step.
If any error occurs in numpy-esque convolve, we can propagate it out instead of unwrapping it and panicking.
ndarray::SliceInfo::try_from only works for [SliceInfoElem; N], where N is up to and including 8. As such, whiltout resorting to Nightly compiler for `generic_const_exprs` feature, we will eventually implement lfilter for Array<_, Dim<[Ix; N]>> instead. Due to some other requirements, lfilter will instead be implemented up to 6 dimensions. See rust-lang:rust#76560.
SpookyYomo
force-pushed
the
lfilter
branch
2 times, most recently
from
60d66c0 to
dd53aeb
Compare
Some warning that came up in `cargo doc`.
Change the function signature to linear_filter first to be more permissive.
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.
![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 November 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.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 14. To receive 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
branch
2 times, most recently
from
ac28ab1 to
d100c40
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
lfilter as FIR first.
Blockers:
Note
Introduces
sci-rs-corewith ndarray-based 1D convolution and integrates a FIRlfilter(N-D, optionalzi, axis support) intosci-rs, updating features/deps accordingly.sci-rs-core):Errortype andResultalias.num_rs::convolveusingndarray-convwithConvolveMode(Full|Same|Valid).sci-rs):lfilterforArray{1..6}(N-D support,axis, optionalzihandling) plus tests.ConvolveModeand use coreconvolveinlfilterinternals.lfilterinsignal::filter::modand refinelfilter_zidocs.sci-rs-core(propagatealloc/std).Written by Cursor Bugbot for commit 396887a. This will update automatically on new commits. Configure here.