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

Bugfix issue#851 #855

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

spranav1205
Copy link
Contributor

Relevant Issue(s)/PR(s) #851

Textbook implementation of impulse response.

Edge cases are 0.5 and phase is taken into account.
The response maybe zero if the width doesn't fall on the sampling times, a warning can be issued for the same.

Eg.
start = 3 width = 5 dt = 4 = [0,1,0.5]
start = 4 width = 4 dt = 4 = [0,0.5,0.5]
start = 1 width = 3 dt = 4 = [0,0.5]

ps. the code has redundancies which I can remove once finalized

spranav1205 and others added 4 commits October 24, 2024 14:40
In this implementation, phase is taken into account. The intensity is 0.5 at the edges.
@matteobachetti
Copy link
Member

@spranav1205 thanks!
A few comments: edge cases should not be plainly 0.5, but account for the exact percentage of zeros and one in a given bin. Also, I suggest implementing tests that demonstrate that the new impulse response is working correctly. I think the formulas you are using will not give the expected results!

@spranav1205
Copy link
Contributor Author

The version I have written (and tested 😭) assigns one if the impulse response lies on a sampling time plainly and 0.5 at the edge. I can write one where the next node (or the previous) is assigned based on how many ones and zeros lie in the bin (essentially the area right?).
Is that what we are looking for??

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.

2 participants