Veto and US time alignment

[siilieva]

Here we can discuss the implementation of the US and the Veto time alignment.

For this first implementation, when one needs the time calibration for a single channel, they have to use the detector method GetCorrectedTime as before.
However, for multi-channel readout (Veto, US, DS horizontal), if an operation is to be performed using all SiPM timings per hit, apply_t_corr flag and SipmDistance parameters are added to all timing getters in the MuFilterHit class. This way the user can call one function per hit that could perform the time calibration for all channels before returning the time value - impact time, delta L/R time.
It is not optimal that there are now 2 ways to return the calibration time - through the detector ( per signle channel at a time!) and through the hit (for multi-channel hits).

[olantwin]

First comment: love the long commit messages! Thanks for taking the time for adding the information!

[olantwin]

The apply_t_corr name is good for the flag. Not too long, but very easy to understand.

Still thinking about a better name for OptForTimeCorrections, not convinced by that name either. Maybe just PrepareTimes? CopyOrCorrectTimes? Especially since the default behaviour is to not correct, we should not have a name that might be misread as applying corrections.

Maybe it would be better to move the conditional into the calling function and then just have a helper function to apply the correction (i.e. the else case of OptForTimeCorrections)?

[siilieva]

PrepareTimes with extension 'PrepareTimesHelper' would match exactly what this function does.

In the current implementation, I use OptForTimeCorrections() to set fTimesHelper[16], which is either a copy of the raw times or the newly calculated corrected times. It seems an elegant way that checks for the apply_t_corr flag once per hit(saves time on ifs).
If one moves the conditional into the Getters, they'll have to call the time correction function channel by channel and repeat a block of if-do in each Getter. But then also, the fTimesHelper array will become redundant, which is an advantage.
I didn't check explicitly what is the computational time/mem. load for each of these two approaches in terms of time. That will decide best. I'll do and provide the estimates.

[siilieva]

The table below summarizes the results if one calls the OptForTimeCorrections once per hit (currently approach on the branch) or the proposed realization calling a function that would apply the correction per individual channel only if apply_t_corr is True. In terms of time the first approach (labelled A in the tables) performs better. In terms of allocated memory, the second approach (B in the tables) performs slightly better as expected since one doesn't need the helper array fTimesHelper.
I'd go with the original approach since the computing time is less, especially in the case the time correction is required (~10% faster per event). On the mem side, the difference is small anyways.

I'm attaching the code+ used to test these two cases, but the main difference is back-end of the MuFilterHit class and is not visible there. The values in the table are best out of 5 trials per case.

• largely based on code by Cris shared in PR#206

import ROOT
import SndlhcGeo
import os,time,psutil

def process_memory():
    process = psutil.Process(os.getpid())
    mem_info = process.memory_info()
    return mem_info.rss

sndgeo = SndlhcGeo.GeoInterface('/eos/experiment/sndlhc/convertedData/physics/2023/geofile_sndlhc_TI18_V5_2023.root')
mufi = ROOT.gROOT.GetListOfGlobals().FindObject("MuFilter")

ch = ROOT.TChain("rawConv")
ch.Add("/eos/experiment/sndlhc/convertedData/physics/2022/run_004541/sndsw_raw-0020.root")
ch.GetEntry(0)
mufi.InitEvent(ch.EventHeader)

N = 1000000
start = time.perf_counter()
mem_before = process_memory()
for i, event in enumerate(ch):
    if i==N-1: break
    for hit in event.Digi_MuFilterHits:
      if hit.GetSystem()!=3:
        hit.GetAllTimes(True, False, 0)
mem_after = process_memory()
stop = time.perf_counter()

print("Seconds per call {}".format((stop - start)/N))
print("consumed total memory:",mem_before, mem_after, mem_after - mem_before)

[olantwin]

Is the difference in memory or time even significant?

Unless you want to perform higher stats tests, I would propose that we go with the simpler solution.

[olantwin]

Tip: Markdown supports table like so:

test	column	a	b
1	2	3	4
r	c	c	l

[siilieva]

The difference in time will become significant for analyses that use large number of events with numerous hits(shower events). The quoted times are 'time per event' and are averaged over 1M events. Then 9% faster per event for millions of events is a lot of time.
The memory wont be an issue in any case.

[siilieva]

Tip: Markdown supports table like so:
test column a b
1 2 3 4
r c c l

I didn't expect that the image of the table would be so ugly pasted here... I had the table already done and wanted to save time.

[olantwin]

The difference in time will become significant for analyses that use large number of events with numerous hits(shower events). The quoted times are 'time per event' and are averaged over 1M events. Then 9% faster per event for millions of events is a lot of time. The memory wont be an issue in any case.

Completely agree that this can quickly add up.

My question was whether the time-difference per event is significant. Execution time can fluctuate a lot depending on the load, data, cache and many other factors. 1M events 5 times might not be enough to establish which is faster (it would be good to know the spread between the 5 runs---at least to have a rough estimate of the uncertainty).

[olantwin]

Tip: Markdown supports table like so:
test column a b
1 2 3 4
r c c l

I didn't expect that the image of the table would be so ugly pasted here... I had the table already done and wanted to save time.

No worries, for next time. And your table is elaborate enough, that it would be a pain to do in markdown.

[siilieva]

The difference in time will become significant for analyses that use large number of events with numerous hits(shower events). The quoted times are 'time per event' and are averaged over 1M events. Then 9% faster per event for millions of events is a lot of time. The memory wont be an issue in any case.

Completely agree that this can quickly add up.

My question was whether the time-difference per event is significant. Execution time can fluctuate a lot depending on the load, data, cache and many other factors. 1M events 5 times might not be enough to establish which is faster (it would be good to know the spread between the 5 runs---at least to have a rough estimate of the uncertainty).

The sigma/mean ~ 3% for both realizations when asking for time alignment. I dont have the estimate for the case when no time corrections are asked for. So the 9% speed gain is legit.

[olantwin]

The sigma/mean ~ 3% for both realizations when asking for time alignment. I dont have the estimate for the case when no time corrections are asked for. So the 9% speed gain is legit.

OK, in that case the difference when not applying corrections seems insignificant and the speedup in the case of applying corrections is small, but probably significant. Thanks!

Then we proceed with A?

[siilieva]

Yes, approach A it is.
Then on the name OptForTimeCorrections, I'd change it to PrepareTimesHelper to match best what the function does.
Meanwhile Andrew is x-checking if the code works as expected.