Skip to content

Fix: ensure impulse response returns at least one bin when width < dt… #914

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sudarshan-21
Copy link

@Sudarshan-21 Sudarshan-21 commented Apr 8, 2025

Bugfix-for-issue #851: Ensure Non-Zero Impulse Response by robustly handling computation for Small Widths in simple_ir

Summary

This pull request addresses an edge case in the simple_ir method where passing a small width value—specifically, one smaller than self.dt—results in a zero-length impulse response. This occurs because the number of bins is computed using integer division, which truncates the result to zero for width < self.dt. Along with a fine-tuning of rounding off of the (width/ self.dt), initially, we were truncating it to the nearest lower int, but for robust handling of all cases math.ceil() can be used.

🔧 Fix

Original Code:

h_ones = np.ones(int(width / self.dt)) * intensity

Updated Code:

import math
h_zeros = np.zeros(math.ceil(start / self.dt))
h_ones = np.ones(math.ceil(width / self.dt)) * intensity

This ensures that for any non-zero width, there will always be at least one non-zero bin in the impulse response.

Bug Description

The impulse response was being generated using:

int(width / self.dt)

If the width is smaller than self.dt, this computation yields zero bins, effectively omitting the impulse response entirely, despite the user intending to generate a small impulse.

Sample Input

self.dt = 1.0
start = 3.0
width = 0.6
intensity = 1.0

Current Output:

[0. 0. 0.]

Expected Output:

[0. 0. 0. 1.]

The expected behavior is that even a short width (e.g., 0.6) should produce a visible impulse response (at least one non-zero value) after the delay.

🔍 Fix and Rationale

📊 Output Comparison Table

Input (width) dt Method num_bins Output
0.6 1.0 int(width / dt) 0 [0. 0. 0.]
0.6 1.0 ceil(width / dt) 1 [0. 0. 0. 1.]

This change ensures accuracy across a broader range of valid width values.

❓ Open Question for Reviewers

I would like to know if the rounding off to a lower integer is intentional or is it fine that we round off the (width/self.dt) to the next larger integer.

✅ Benefits

  • Fixes a silent bug for narrow impulse widths
  • Enhances robustness of the impulse generation
  • More faithful representation of user-specified widths

✅ Checklist

  • Fixes the edge case for small widths
  • Preserves compatibility for width >= dt
  • Tested with sample inputs and verified output consistency

Let me know if further refinements or configuration support is required!

… and fine tuning the calculation for precise results
@ankitkhushwaha
Copy link
Contributor

Hey @Sudarshan-21 , thanks for the pr.
But can u write a concise detail about the bugfix. It looks like u have use LLMs to write the description.
Please see #891

@Sudarshan-21
Copy link
Author

Sudarshan-21 commented Apr 8, 2025

Sure, @ankitkhushwaha, I was facing difficulty in generating the tables in .md, so I took help from a LLM. I have now edited it manually, can you please review it. Also, I have a "question for the reviewers" section, which I would like the maintainers' opinion on. It would be helpful if you can give your input.

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