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

Simulator.simple_ir: width should always be > self.dt #851

Open
matteobachetti opened this issue Oct 17, 2024 · 4 comments
Open

Simulator.simple_ir: width should always be > self.dt #851

matteobachetti opened this issue Oct 17, 2024 · 4 comments
Labels
bug It's a bug! Unexpected or unwanted behavior. help wanted We need additional help with these issues!

Comments

@matteobachetti
Copy link
Member

Description of the Bug

The impulse response here is a simple series of ones that come after a certain delay. The user inputs a width of the series of ones. The number of ones is calculated as

        # Define constant impulse response
        h_ones = np.ones(int(width / self.dt)) * intensity

If width is smaller than self.dt, this operation gives no bins with one.

Steps/Code to Replicate the Bug

Call simple_ir with width = 0.1 * self.dt, e.g.

Expected Results

The list in ouput should be something like [0, 0, 0, 1]

Actual Results

It's [0, 0, 0]

@matteobachetti matteobachetti added bug It's a bug! Unexpected or unwanted behavior. help wanted We need additional help with these issues! labels Oct 17, 2024
@spranav1205
Copy link
Contributor

Hey!
So if I understand correctly, we want to mask our impulse train (that has a frequency self.dt), with a width starting at some start time?
In that case, we need to consider the phase. How about this

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

@matteobachetti
Copy link
Member Author

@spranav1205 I guess one could just use min to avoid that the "ones" array has no elements

@spranav1205
Copy link
Contributor

Yes, that would certainly work. But I don't understand one thing. Say start = 1, width = 2, and self.dt = 4. The impulse should not appear while sampling, right?

Would you like me to make a PR using min?

@matteobachetti
Copy link
Member Author

Yes, that would certainly work. But I don't understand one thing. Say start = 1, width = 2, and self.dt = 4. The impulse should not appear while sampling, right?

The impulse should always appear, having a 0 impulse response is always useless :)
In this particular case, there is an additional complication, because the start is 1 which is smaller than dt as well. We can modify the method so that it also accounts for fractional contributions (e.g., the impulse response starts at 1 and lasts 2, but the dt is 4, so the impulse response will just be [0 + 2/4 + 0] = [0.5]), or otherwise decide that it just fails if the start is less than one bin, and it always have at least one bin.

e.g.

  • start=4, width=2, dt=4 -> [0, 1] # max(width/dt, 1) = 1
  • start=3, width=2, dt=4 # FAIL because start < dt
  • start=4, width=2, dt=2 -> [0, 0, 1]

or

  • start=4, width=2, dt=4 -> [0, 0.5]
  • start=3, width=2, dt=4 -> [0.25, 0.25]
  • start=4, width=2, dt=2 -> [0, 0, 1]

Would you like me to make a PR using min?
Actually "max" as in the example above. I think the previous set of solutions I wrote are the best for a "simple impulse response"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug! Unexpected or unwanted behavior. help wanted We need additional help with these issues!
Projects
None yet
Development

No branches or pull requests

2 participants