Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added q value parameter to be passed to filter functions #515

Conversation

xwillxw
Copy link
Contributor

@xwillxw xwillxw commented Oct 6, 2023

I have added a 2nd parameter to the filter functions low_pass, high_pass, to_low_pass, and to_high pass; allowing a q value to be passed along with the frequency.

I have added a 2nd parameter to the filter functions low_pass, high_pass, to_low_pass, and to_high pass; allowing a q value to be passed along with the frequency.
@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 6, 2023

this is intended as a solution to #514

Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there, thanks for the contribution, I am just passing along and had some thoughts hope you don't mind me adding them as a review:

I personally have a vague idea what a q value is, but I do know high/low pass and freq. How would you feel about adding doc/comments that specify a good default q value?

And/Or we could change the proposed API, make it an Option defaulting to a good default. Yet another alternative would be the builder pattern, though that might be overkill.

Finally, this would change the public API (adding a parameter). That would require every downstream user to fix their code adding the parameter. Maybe we could add another method? Something along the lines of high_pass_with_q?

@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 6, 2023

I agree that changing the existing method could cause problems.
I could update the proposed API to involve an option defaulting to 0.5, the original default q value within new methods such as high_pass_with_q(). However, this is not a smart solution at all. Creating a new method in order to perform exactly the same function but with an extra parameter is not an efficient solution to the issue. This should be kept in mind when creating similar functions in order to avoid technical debt like this.

@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 6, 2023

I have added secondary methods which are the same as low_pass, high_pass, to_low_pass and to_high_pass titled low_pass_with_q, etc.
They allow the q value to be passed to these functions too, changing the bandwidth of said filters.

@dvdsk
Copy link
Collaborator

dvdsk commented Oct 6, 2023

Creating a new method in order to perform exactly the same function but with an extra parameter is not an efficient solution to the issue.

I think I have got a good solution for that. Lets make the implementation of the non q versions call the with_q versions with as q value 0.5. The compiler will inline the call so this won't make the code slower.

The low_pass, etc. now just call low_pass_with_q while also passing  the original default q value of 0.5
@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 6, 2023

I have implemented this correction. low_pass(), etc. now just call low_pass_with_q(),etc. and pass the cutoff frequency along with the original default Q value of 0.5. This means that previously written code will not be broken and downstream users will not have to update their code, however, the option to change the q value is now there if required.

@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 6, 2023

I believe this could be better implemented using a default instead of a secondary method. I will implement this change

@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 6, 2023

Upon revision. that would not be possible. I believe this should be pulled to master as the with_q functions work as desired.

@xwillxw xwillxw requested a review from dvdsk October 7, 2023 21:34
Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I found some small stuff but that should be easy to fix.

After that you should ask est31 for a review. They are a maintainer and can merge this. I am only trying to help them out a bit.

Ps; its perfectly normal to have little errors like this, I have it too. You read over it yourself without noticing.

src/source/blt.rs Outdated Show resolved Hide resolved
src/source/mod.rs Show resolved Hide resolved
src/source/mod.rs Show resolved Hide resolved
@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 7, 2023

My apologies for the rookie mistakes. I am fairly new to rust as a programming language. I will submit the pull request for review. You have been a massive help, thank you.

@xwillxw
Copy link
Contributor Author

xwillxw commented Oct 7, 2023

@est31 could this please be merged? Thanks.

@est31 est31 merged commit 98bae98 into RustAudio:master Oct 17, 2023
10 checks passed
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