Skip to content

Conversation

@SpookyYomo
Copy link
Contributor

@SpookyYomo SpookyYomo commented Oct 21, 2024

This PR aims to implement Firwin.

Blockers before merging:


Original Message:

Currently opening as a draft PR to have eyes on the code before it grows even larger into eventually implementing firwin.

Questions I guess I need answers about specific to this repository to ensure the PR is decent:

  1. What exactly in a function is allocating/otherwise for me to determine if the function name should be *_st or *_dyn?
  2. File structure... As a first time contributor, it feels like it's just having many files and then being flattened by mod.rs to follow the scipy's submodule convention.... Individual files otherwise don't really make sense, and finding a function is best done either via grepping or through the [Source] button as created by cargo docs.
  3. Doc's index.html: Maybe we should structure the Doc abit more clearly similar to Scipy's documentation where there's a clear header of Convolution, B-splines, Fitting, ...? I think this is achieveable with //!? Or I might be mistaken.
  4. Unsafe code: I noticed some other functions using unsafe { F::from(-1).unwrap_unchecked) } or something similar. Is this the norm for "constants" which we can guarantee to be safe, and so I too should be doing this?

I will continue to force-push onto this branch with any suggestions such that each commit is consistent with its commit title and message.


Note

Implements FIR filter design via firwin_dyn with validation and scaling, adds comprehensive window functions and Kaiser utilities, introduces an Error type, and updates signal module exports/docs.

  • Filter design (signal/filter/design):
    • firwin_dyn: FIR filter design with argument validation, window application (custom or width-based Kaiser), scaling, and extensive tests.
    • Kaiser utilities: add kaiser_beta, kaiser_atten, kaiserord with tests.
  • Windows (signal/windows):
    • Add GetWindow trait, Window enum, and get_window builder.
    • Implement windows: Boxcar, Triangle, Blackman, Hamming, Nuttall, Kaiser, GeneralCosine, GeneralGaussian, GeneralHamming (each with tests and docs).
  • Errors:
    • New error::Error enum (InvalidArg, ConflictArg); export via lib.rs.
  • Module/docs updates:
    • Expose signal::windows and enhance docs; integrate new design modules in signal::filter::design and adjust filter/wave docs.

Written by Cursor Bugbot for commit 7760754. This will update automatically on new commits. Configure here.

@SpookyYomo SpookyYomo force-pushed the firwin branch 2 times, most recently from c042c55 to cab3736 Compare October 22, 2024 10:30
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.
@SpookyYomo SpookyYomo force-pushed the firwin branch 2 times, most recently from 060cf5b to 21010f8 Compare November 23, 2024 21:42
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.
@SpookyYomo
Copy link
Contributor Author

SpookyYomo commented Dec 8, 2024

a92506d I have added error types after looking through how the Rust ecosystem generally does errors. This might still not be ideal and needs more work.

We should leave panics instead to Python-facing side of the library.

@SpookyYomo SpookyYomo force-pushed the firwin branch 2 times, most recently from 0e90be5 to a1783ca Compare December 8, 2024 01:48
@SpookyYomo

This comment was marked as outdated.

@SpookyYomo
Copy link
Contributor Author

SpookyYomo commented Dec 8, 2024

4598cbd There's still some redundancy in argument specification of Firwin. But I'm not sure how to resolve it.

Nonetheless it behaves to expectation now.

@SpookyYomo
Copy link
Contributor Author

Forced push with rebase since Bessel trait has been merged.

@SpookyYomo
Copy link
Contributor Author

Forced push with rebase since main has since bumped.
Also removed special-fun dependency since it was not removed in the last rebase.

@SpookyYomo
Copy link
Contributor Author

SpookyYomo commented Jun 19, 2025

Variadic arguments

There is https://crates.io/crates/variadics to resolve the struct-as-arg conundrum if we wish for a more pythonic interface, but I still believe that we should only keep the pythonic interface at the python binding level.
Related PR: #85

Design of interfaces

https://purplesyringa.moe/blog/the-expression-problem-and-rust/
The author argues on operational domains and data type as returns.
The reality is that rust intentionally does not intend to have implicit, hidden information
at a callsite, so an arg-struct pattern makes things more explicit.

@SpookyYomo
Copy link
Contributor Author

SpookyYomo commented Sep 9, 2025

An alternative is as done exactly in here,
but it also suffers the issue of variadic function lengths.

edit: The linked crate is almost certainly entirely AI generated, but it does show how
we could build ourselves into a corner.
edit 2: someone else points out an identical issue the exact same problem which Arg-Struct solves

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@SpookyYomo SpookyYomo force-pushed the firwin branch 2 times, most recently from 7129e87 to 4f8db57 Compare September 22, 2025 05:30
cursor[bot]

This comment was marked as outdated.

@SpookyYomo SpookyYomo force-pushed the firwin branch 2 times, most recently from ee854e1 to f96e406 Compare September 22, 2025 05:56
Scipy returns a value error in this case.
cursor[bot]

This comment was marked as outdated.

Wrong argument was being signalled in the message.
Also add a unit test.
@cursor
Copy link

cursor bot commented Dec 21, 2025

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on January 14.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

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.

3 participants