Filter calculation tools

[yomach]

Hi, take a look at this and please review it :)

It's without any tests and without the changelog/readme changes.

It's also mostly without any hardware limitations (besides the highpass), we need to decide if we want to include them or not. And if so, how to deal with changing limitations.

[github-actions]

Unit Test Results

382 tests   379 ✔️  44s ⏱️
    1 suites      3 💤
    1 files        0 ❌

Results for commit 62c87a7.

♻️ This comment has been updated with latest results.

[TheoLaudatQM]

I like the idea of finally putting all of these calculations on the Github very much! 💯
I looked at the highpass_correction and exponential_correction which I know and it looks good. Do you know the maximum high pass time constant that we can correct based on the resolution of the FIR taps? Is it on 17 or 21 bits?
I am not familiar with the bounce_and_delay_correction and the filters concatenation, but I trust you :)

[yomach]

I like the idea of finally putting all of these calculations on the Github very much! 💯 I looked at the highpass_correction and exponential_correction which I know and it looks good. Do you know the maximum high pass time constant that we can correct based on the resolution of the FIR taps? Is it on 17 or 21 bits? I am not familiar with the bounce_and_delay_correction and the filters concatenation, but I trust you :)

Thx!
It was actually planned for a long time...

I want to remove the HW limit from the main function (maybe add it as another function) because:

1. We're not even sure what it is (see my slack message)
2. This will change between versions...

[TheoLaudatQM]

I see, regarding the HW limitation I think it is now clear that currently the FIR taps are on 25bits and the IIR on 17bits. Maybe we can add flag to discriminate between the QOP (or qm-qua) versions and make these limitations explicit so that the users will know what is going on and if the given taps are accurate or not, what do you think?

[TheoLaudatQM]

Hey @yomach, I like this one a lot and many customers are actually asking for such a tool.
What is preventing us from pushing it forward?

[yomach]

Not much actually. I guess that we need to:

1. Remove the hardware limitations from highpass_correction - make all functions return the ideal ones by default, and then have a separate function that can accept filters and change them according to the hardware limitations (and, it will accept an enum with a version in the future). That's at least the method I had in mind, Open to suggestions*.
2. Add some tests
3. Add to changelog & readme. Go over docstrings (I think I wrote them alright).

Do you have time to do it?

• Maybe we can also have the main function accept an optional qop_version enum that will apply the hardware constrains if given, and return ideal ones if not.

[TheoLaudatQM]

Cool!
Yes I can definitely give it a shot :)

[yomach]

Looks good so far!

[TheoLaudatQM]

I fixed the function as we discussed and added some tests.
I have a question regarding the delay: you said:

if delay is not a multiple of the sampling rate, multiple FIR taps will be created.

But even with delay=11 and Ts=2, I get 111 taps resulting in an error so I wanted to raise an error if delay is not a multiple of the sampling time, what do you think?

[yomach]

I fixed the function as we discussed and added some tests. I have a question regarding the delay: you said:

if delay is not a multiple of the sampling rate, multiple FIR taps will be created.

But even with delay=11 and Ts=2, I get 111 taps resulting in an error so I wanted to raise an error if delay is not a multiple of the sampling time, what do you think?

Ts = 2 is not a real scenario 😅 (Should be Ts = 0.5 for the OPX1000, in the future).
The relevant usecase, right now, is Ts=1 and delay=10.25.
We shouldn't raise an error, but we should allow trimming the number of taps (as part of applying the hardware limitation).
Something such as:

max_fir_taps = 44 - 7*len(feedback_taps)
if len(feedforward_taps)>max_fir_taps :
    feedforward_taps = feedforward_taps[:max_fir_taps ]

We can also add a message about it. I'm thinking something such as: summing the abs of the discarded FIR taps. If it's more than 0.01 (1%), give a warning, otherwise, just a debug message.
This is relevant because doing delay=1.5 would not really work (discarded taps will be >1%, probably ~10%), but delay=10.5 should be ok.

[TheoLaudatQM]

Good point!
Although it is the same with Ts=1 and delay=10.5ns. It works more or less okay if no other fir and iir taps are needed, otherwise the outputted signal is crap because we don't have enough taps available I guess.
On the other hand, I never used it and I don't know how many customers do...

I also initiated the hardware limitation function, but I am not sure about the best implementation and about the correct values...

[yomach]

I think that the errors/warning I suggested above would cover this "crap signal" scenario, WDYT?
We can discuss the hardware limitation.

[TheoLaudatQM]

If we raise an error, then it will be solved, but if we only raise a warning, then the user will be notified about it but the OPX will still output something that can be harmful for the device...
Maybe I can add a safety option set to True by default that will raise an error and the user can set it to False if he still wants to output the signal. WDYT?

[yomach]

If we raise an error, then it will be solved, but if we only raise a warning, then the user will be notified about it but the OPX will still output something that can be harmful for the device... Maybe I can add a safety option set to True by default that will raise an error and the user can set it to False if he still wants to output the signal. WDYT?

I don't think it can harm the device. Removing taps makes the OPX send smaller signals.
But I'm ok with the default being sending an error.