Bug in time lapse drs4 correction

[maxnoe]

This is the current code in the master for updating the drs4 read out times for the extra readout of the first capacitors:

https://github.com/cta-observatory/cta-lstchain/blob/4da8e3a55bd3f80d8b285b9c5ee130485ecea165/lstchain/calib/camera/r0.py#L493-L502

If I understood correctly, this code behaves wrong when the first capacitor is is in the range where start < 4096 but end > 4096, since then the slice is empty:

In [1]: import numpy as np

In [2]: 
In [2]: time_diff =np.arange(4096)

In [3]: first_cap = 3094

In [4]: first_cap = 3068

In [5]: start = first_cap + 1024

In [6]: end = first_cap + 1024 + 12

In [7]: first_cap, start, end
Out[7]: (3068, 4092, 4104)

In [8]: time_diff[start % 4096:end%4096]
Out[8]: array([], dtype=int64)

[maxnoe]

So I think in some cases (roughly 12 / 4096 / 2) the readout time is not updated although it needs to be update.

[jsitarek]

Hi @maxnoe Thanks for catching it, it really looks like a bug. The simplest solution that I can think of is:

start = (int(first_cap) + size1drs ) % size4drs end = start + 12

last_time_array[nr_module, gain, pix, start:end] = time_now if end > size4drs4: last_time_array[nr_module, gain, pix, 0:end-size4drs] = time_now

The first assignment apparently goes out of range if end > 4096, but testing on simple arrays this does not seem to be a problem and the assigment only goes up to end of the array.

@pawel21 , can you test it on your standard pedestal files to see that there is no nasty side effect ?

[maxnoe]

Since this is in numba, a simple loop and applying the modulus inside the loop would also be fine.

[maxnoe]

I am currently refactoring this code over at ctapipe_io_lst: https://github.com/cta-observatory/ctapipe_io_lst/pull/47 and would be very happy if you and @pawel21 could review as soon as I'm done. Will drop you a note, when it's ready to review.

[jsitarek]

right, if there is no overhead, this is even cleaner

[maxnoe]

can you test it on your standard pedestal files to see that there is no nasty side effect ?

What is that "standard pedestal file" and how does the test go? We now have a webserver for protected test data and the drs4 code is lacking test coverage, so it would be great to use this file for unit testing.

@pawel21 can you tell me the run number / other information required and what exactly you are testing so I can transform that into an automatic test?

[pawel21]

Dear @maxnoe sorry for late reply. We don't have "standard pedestal file", but I often use Run01611.000 to test drs4 correction. Run 1611 was without drs4 pedestal correction. Thank you for your refactoring, I will try look on it, but I am at LST/MAGIC shift now and this time is very busy.

[maxnoe]

@pawel21 Can you explain what you mean with "testing"? What are you looking at to verify it works correctly?

[maxnoe]

@jsitarek @pawel21 I have an additional question concerning the second part (the elif >= 1013)

The code does not update exactly 12 cells as stated in the comment. Is this correct? Why is end not simply start + 12 here?

[jsitarek]

Hi @maxnoe the second part of code (that is updating less then 12 slices) is correct. Both conditions together mean to update up to 12 slices, but as long as it does not cross to the next channels of DRS4 (to the next 1024 slices) The description however can be improved.