Project

General

Profile

Actions

Sim-Development #2131

open

Express digi times relative to the time slice

Added by Felix Weiglhofer about 1 year ago. Updated 11 months ago.

Status:
Resolved
Priority:
High
Target version:
Start date:
05/20/2021
Due date:
% Done:

100%

Estimated time:
Spent time:

Description

Steps to reproduce:

  1. Add the line assert(time >= 0); in function CbmStsDigitize::CreateDigi (in file sim/detectors/sts/CbmStsDigitize.cxx:149)
  2. Rerun test: cd build && make && ctest -R "run_s100e_digi_ev" --output-on-failure .
  3. Test run_s100e_digi_ev will fail at the assert from step 1.

Files

trddigi_old.png (11 KB) trddigi_old.png TRD digi time before MR Volker Friese, 09/08/2021 12:14 PM
trddigi_new.png (10.8 KB) trddigi_new.png TRD digi time after MR Volker Friese, 09/08/2021 12:14 PM
tracktime_old.png (12.2 KB) tracktime_old.png STS track time before MR Volker Friese, 09/08/2021 12:15 PM
tracktime_new.png (12.3 KB) tracktime_new.png STS track time after MR Volker Friese, 09/08/2021 12:15 PM

Related issues

Blocks Framework - Development #2058: Update size of STSDigi to 8 byteFeedbackFelix Weiglhofer03/29/2021

Actions
Actions #1

Updated by Felix Weiglhofer about 1 year ago

Actions #2

Updated by Felix Weiglhofer about 1 year ago

  • Description updated (diff)
Actions #3

Updated by Volker Friese about 1 year ago

  • Status changed from New to In Progress
  • Assignee set to Felix Weiglhofer

This is a know feature arising in event-by-event simulation. The reason is the following:

In event-by-event simulation, the MC event time is set to zero for each event. Now the time-of-flight from the event vertex to the STS is only several ns (the last STS station is about 1 m from the target). The analogue reponse of the STS is prompt (< 1 ns). The digital time resolution of the readout ASIC is assumed to be 5 ns (Gaussian). As a result of all of this, negative digi times can happen.

In the time-based simulation, we prevent such cases by letting the run start at t = 1000 ns, such that negative digi times do not appear. This is because the first time slice starts at t = 0, thus it is avoided that digis are lost (not sorted into any time slice). In event-by-event, this is not an issue, since the "time slice" in this case has no fixed time interval, but comprises all digis from this event anyhow.

It would be a rather straightforward solution to do something similar for event-by-event, i.e., set the MC event time for each event to e.g. t = 1000 ns instead of t = 0. However, the whole problem has another aspect, which is that in (time-based) simulation, there is no time slice from the start to set the time reference. Instead, digis are produced (and stored into a buffer) continuously, and then time slices are filled from this DAQ buffer. That means that the digi time is set before adding it to a time slice. The dynamical range for the digi time thus has to cover the entire run, not only a single time-slice interval. We currently use a long int (64 bit) for the time coordinate, which gives a range of some 600 years in nanosecond precision.

Reducing the time representation to 32 bit, as we envisage with CbmTimeStamp, obviously does not fit this scheme - it is good for a single time slice, but not for an entire run. So, the simulation scheme would have to be changed before we can change the StsDigi class.

Actions #4

Updated by Felix Weiglhofer about 1 year ago

Why not set the Digi time only when filling the time slice? CbmDigitize already stores the global timestamp anyway alongside the digi. And we can make the timestamp relative to the time slice as well at that point.

Actions #5

Updated by Felix Weiglhofer about 1 year ago

  • Blocks Bug #2152: mCBM pulser does not have a valid STS address added
Actions #6

Updated by Felix Weiglhofer about 1 year ago

  • Blocks deleted (Bug #2152: mCBM pulser does not have a valid STS address)
Actions #7

Updated by Volker Friese about 1 year ago

There are two stages of buffering in the digitization. The first is in CbmDigitize, where the analogue signals are buffered between events to allow interference within the dead time. Here you are right, the time of the analogue signal is stored in an analogue way (with a Double_t, I think). When the signal can be safely digitized (further interference is excluded), digis are created with a digitized time (still global).

The second buffering stage happens in CbmDaq, where digis from all detectors are sent to and are sorted into time slices. Of course, once it is known which time slice the digi belongs to, the time information can be reduced (made relative to that time slice), but how to do this before? The detector digitizers do not know about time slices.

Actions #8

Updated by Felix Weiglhofer about 1 year ago

I would add an option to pass a digi plus the global timestamp from the detector digitizer to the digitize base class and use that as the key for the daq buffer instead of the digi timestamp. Then during the second stage calculate and set the relative time of the digi. This should avoid storing the global timestamp inside the digi.

Actions #9

Updated by Volker Friese about 1 year ago

Indeed, that appears to be the proper solution. So what you propose is to change the method

void SendData(Digi* digi, CbmMatch* match = nullptr)

to

void SendData(Double_t time, Digi* digi, CbmMatch* match = nullptr)

and, when finally filling the digi from the buffer to the output time slice, calculate the digi time relative to the time slice start (probably in CbmDigitize::FillTimeSlice).

That's a moderate-size project, not difficult, but requires intervention in all concrete digitizer classes (deriving from a concretization of CbmDigitize), wherever they call CbmDigitize::SendData().

Actions #10

Updated by Volker Friese about 1 year ago

  • % Done changed from 0 to 70

As you demonstrate in MR 395, the changes to the digitizers are indeed minute. Most can be handled at framework level (i.e., in the CbmDigitize base class). Very good.

As discussed in the software meeting on 1 July 2021, the consequences are that also the times of derived/reconstructed objects (clusters, hits, tracks, events) will be relative to the respective time slice. I will put this into the news, giving developers and users the chance to object. I do not see immediate problems arising from this, other than simple data browsing with the ROOT TBrowser will not give you the full time axis. When comparing data objects from different timeslices (e.g. in the timelice overlap region), one has of course to expand to an "absolute" time.

Also, as pointed out yesterday, we have to take care of the event-by-event case (timeslice type kEvent) and the case when all data are in one timeslice (timeslice type kFlexible). In these cases, there is no well-defined start time of the time slice.

Actions #11

Updated by Volker Friese about 1 year ago

My proposal is:

  • For event-by-event (CbmTimesliceType::kEvent), the proper time reference is the event start time. We can avoid negative digi times by letting each event start at t = 1000 ns.
  • For time-based simulation with all data into one timeslice (CbmTimeSliceType::kFlexible), the proper reference is the run start (t=0). Negative digi times are already now avoided by starting the first event at t = 1000 ns. They can still appear for noise digis (which are continuously produced), but then there is no harm in neglecting those with negative times. Of course, we can overrun the dynamic time range in this way, but this is anyhow a sort of debug mode, used to circumvent the issue of losses at the time slice border for small time slices.

If you agree to that, I can do the necessary changes.

Actions #12

Updated by Volker Friese about 1 year ago

  • Subject changed from StsDigitization creates Digis with negative timestamp. to Express digi times relative to the time slice
Actions #13

Updated by Volker Friese about 1 year ago

  • Tracker changed from Bug to Sim-Development
  • Project changed from CbmRoot to Simulation
  • Target version set to DEC21
Actions #14

Updated by Pierre-Alain Loizeau about 1 year ago

In fact one thing which came to my mind only today is that most "legacy" unpackers were using "time-in-run" instead of "time-in-timeslice" for the Digis, thanks to the "Epoch cycle" or "TS_MSB cycle" (at least those I started: STS, MUCH, TOF).

So if no strong objection to the new convention is voiced by the Physics side, we should have a look there during the creation of the new ones and make sure that we also follow it, so that we do not have surprises later in reconstruction.
That may pass by a mandatory (by specification and not construction, as we do not use a base template) variable "timeslice_start_time" which should be the first one to be filled by each unpacker when opening the first MS in the timeslice

Actions #15

Updated by Volker Friese 11 months ago

  • Priority changed from Normal to High

On the data side, all unpackers by now set the digi time w.r.t. the time slice start.

For the simulation side, Felix has proposed the necessary changes in MR 395. I tested this with 10 events in the electron setup with the standard simulation (event-by-event) an reconstruction chain against the current HEAD.

Although in particular for event-by-event simulation, no changes are expected, I do see a large difference in the number of found STS tracks (6747 tracks with the MR against 4478 in the HEAD). The numbers of digis, clusters, and hits are identical in both cases.

So, we have to check L1 tracking. Valentina, please have a look. The MR source is https://git.cbm.gsi.de/fweig/cbmroot; branch digitizer-fix.

Actions #16

Updated by Volker Friese 11 months ago

  • Assignee changed from Felix Weiglhofer to Valentina Akishina
Actions #17

Updated by Valentina Akishina 11 months ago

Dear all,

I checked the issue and indeed new digi time was breaking the track finder performance.
Namely almost all tracks with MVD hits were split into two parts, doubling total number of tracks.

The reason for that is that we were setting internally MVD hit time to zero and the time error to a large value, so that it has no influence on the reconstructed track time.
(Some time ago the MVD hit time and error was set negative after local reconstruction.)
After changing the digi time it was breaking the tracks since the difference between MVD and STS hit time was larger than the estimated errors.

I turned off manually the usage of MVD hit time in the track finder and this fixed the issue.
https://git.cbm.gsi.de/computing/cbmroot/-/merge_requests/477

Clone level has decreased from about 50% to normal values.

Best regards,
Valentina

Actions #18

Updated by Volker Friese 11 months ago

Valentina Akishina wrote:

Dear all,

I checked the issue and indeed new digi time was breaking the track finder performance.
Namely almost all tracks with MVD hits were split into two parts, doubling total number of tracks.

The reason for that is that we were setting internally MVD hit time to zero and the time error to a large value, so that it has no influence on the reconstructed track time.
(Some time ago the MVD hit time and error was set negative after local reconstruction.)
After changing the digi time it was breaking the tracks since the difference between MVD and STS hit time was larger than the estimated errors.

First, thanks for the quick fix.

However, I do not understand your explanation. In the event-by-event simulation which I tested, the event start time is always zero, so the time of the StsDigis is < 10 ns. This does not change with the MR.

In time-based simulation, there is no MVD, so the problem should not be there.

Actions #19

Updated by Volker Friese 11 months ago

  • Assignee changed from Valentina Akishina to Volker Friese

However, I do not understand your explanation. In the event-by-event simulation which I tested, the event start time is always zero, so the time of the StsDigis is < 10 ns. This does not change with the MR.

Sorry, I have to correct myself. The changes in the MR include that the event time is 1000. in the event-by-event mode; this was 0. before (done so to avoid negative digi times which the new StsDigi format will not support). So, I can understand your explanation.

I have tested with the updated MR and find (approximate) agreement in the number of StsTracks (4532 versus 4478, only a slight increase by some 1 %). Valentina, do you understand this 1 %?

Side remark: Sadly, the TRD and PSD digis do not recognize the changes event time; their times are still around 0. Same holds then also for the TRD hits, consequently. The PSD hits have no time attribute. This will have to be followed up in a separate issue.

I will proceed now testing with time-based simulations.

Actions #20

Updated by Volker Friese 11 months ago

I have tested with time-based simulations: same 10 MC events, event rate 10^5, time-slice length 50000 ns (in order to have several time slices in the output). Observations:

  • The PSD digitizer cannot be used in time-slice mode, it always produces digi times close to zero and seems to completely ignore the event time. It even leads to a crash in the digitization.
  • STS, RICH and TOF digitizers appear to work properly; same number of produced digis before and after the changes of the MR. The digi times seem proper.
  • The TRD digitizer seems fishy (see the attached plots). In the head version, the TRD digi times look correct; with the MR, they have much too small values (< 1000 ns). Interestingly, the TRD digi time distribution changes if I choose a large time slice (1e9 ns), such that all data are in one timeslice. But they still are not correct (< 3000 ns). And the number changes slightly (53071 with time slice 50000 ns, 53009 with time slice 1e9 ns). So here, we definitely have to have a look.
  • STS tracking looks roughly ok from the number of found tracks (4107 vs 3795 before the MR, an increase by some 8 %). Interestingly, with the large timeslice, I get 4077 tracks, slightly less, which I do not understand. Was before 3814, slightly more than with small timeslices, which I can understand if the hits of some tracks are split between different time slices. Completely wrong is the track time (see attached plots), which after the MR is mostly zero.

So, for this MR, we should try to fix the TRD issue. The PSD digitizer is not time-based capable independent from the MR. The tracking issues must be solved outside this issue / MR.

Actions #21

Updated by Sergey Gorbunov 11 months ago

Volker, there is a couple of bugs introduced in MR 477 https://git.cbm.gsi.de/computing/cbmroot/-/merge_requests/477
They are fixed in MR 482 and 483.

https://git.cbm.gsi.de/computing/cbmroot/-/merge_requests/482
https://git.cbm.gsi.de/computing/cbmroot/-/merge_requests/483

After applying 477 482 483, the tracker performance on electrons is a bit different (better) than it was before, this is ok.

Volker Friese wrote:

I have tested with the updated MR and find (approximate) agreement in the number of StsTracks (4532 versus 4478, only a slight increase by some 1 %). Valentina, do you understand this 1 %?

Where do you see this 1% performance variation? Is it a variation after applying 477 (this is now fixed and understood), or a variation after applying the new digitizer to 477 (it shouldn't be the case)?

Actions #22

Updated by Volker Friese 11 months ago

Where do you see this 1% performance variation? Is it a variation after applying 477 (this is now fixed and understood), or a variation after applying the new digitizer to 477?

I tested MR 395 (which already includes MR 477, but not the fixes in MR 482 and 483).

Actions #23

Updated by Sergey Gorbunov 11 months ago

Volker Friese wrote:

Where do you see this 1% performance variation? Is it a variation after applying 477 (this is now fixed and understood), or a variation after applying the new digitizer to 477?

I tested MR 395 (which already includes MR 477, but not the fixes in MR 482 and 483).

Yes, but what do you compare: 477 to the previous cbmroot, or 477 to 477+ new digitizer?

Actions #24

Updated by Volker Friese 11 months ago

My reference is the master before 395, head is aff595e4.

Actions #25

Updated by Sergey Gorbunov 11 months ago

Volker Friese wrote:

My reference is the master before 395, head is aff595e4.

ah, ok. 477+482+483 introduce small changes, the new digitizer should not change anything.

Actions #26

Updated by Pascal Raisig 11 months ago

Volker Friese wrote:

I have tested with time-based simulations: same 10 MC events, event rate 10^5, time-slice length 50000 ns (in order to have several time slices in the output). Observations:

  • The TRD digitizer seems fishy (see the attached plots). In the head version, the TRD digi times look correct; with the MR, they have much too small values (< 1000 ns). Interestingly, the TRD digi time distribution changes if I choose a large time slice (1e9 ns), such that all data are in one timeslice. But they still are not correct (< 3000 ns). And the number changes slightly (53071 with time slice 50000 ns, 53009 with time slice 1e9 ns). So here, we definitely have to have a look.

Hi Volker, all,
I had a look at the TRD digitzer. I think the problem is not in the digitizer itself, it is in the

SetTime(Double_t t) 
function of the CbmTrdDigi (see CbmTrdDigi.cxx line 246). I think this was never used for 1D digis. It tries to store the time as multiple of the asic clock in fTime, which is valid in the 2D case, however, for the 1D case we store the absolute time in ns in fTime.
Here is a potential fix (to be tested within the setting of the MR):
//_________________________________________________________________________________
void CbmTrdDigi::SetTime(Double_t t)
{
  switch (GetType()) {
    case eCbmTrdAsicType::kFASP: fTime = ULong64_t(TMath::Ceil(t / Clk(GetType())));
    case eCbmTrdAsicType::kSPADIC: fTime = static_cast<ULong64_t>(t);
    case eCbmTrdAsicType::kNTypes: return;
  }
}

Shall I prepare a commit with the fix or will one of you guys add it to the open MR?

Cheers,
Pascal

Actions #27

Updated by Pascal Raisig 11 months ago

Pascal Raisig wrote:

Volker Friese wrote:

I have tested with time-based simulations: same 10 MC events, event rate 10^5, time-slice length 50000 ns (in order to have several time slices in the output). Observations:

  • The TRD digitizer seems fishy (see the attached plots). In the head version, the TRD digi times look correct; with the MR, they have much too small values (< 1000 ns). Interestingly, the TRD digi time distribution changes if I choose a large time slice (1e9 ns), such that all data are in one timeslice. But they still are not correct (< 3000 ns). And the number changes slightly (53071 with time slice 50000 ns, 53009 with time slice 1e9 ns). So here, we definitely have to have a look.

Hi Volker, all,
I had a look at the TRD digitzer. I think the problem is not in the digitizer itself, it is in the[...] function of the CbmTrdDigi (see CbmTrdDigi.cxx line 246). I think this was never used for 1D digis. It tries to store the time as multiple of the asic clock in fTime, which is valid in the 2D case, however, for the 1D case we store the absolute time in ns in fTime.
Here is a potential fix (to be tested within the setting of the MR):
[...]

Shall I prepare a commit with the fix or will one of you guys add it to the open MR?

Cheers,
Pascal

One question where I am not sure how to handle it, is the type of the passed time in this case. I think it would be better to switch here to uint64 to have enough bits for the absolute time and anyhow we do not store the digits after the floating point. Is there a reason I do not see to keep this as a double?

Actions #28

Updated by Volker Friese 11 months ago

Pascal Raisig wrote:

Shall I prepare a commit with the fix or will one of you guys add it to the open MR?

Thanks for the fix. I will test it and include it into the MR.

Actions #29

Updated by Volker Friese 11 months ago

Pascal Raisig wrote:

One question where I am not sure how to handle it, is the type of the passed time in this case. I think it would be better to switch here to uint64 to have enough bits for the absolute time and anyhow we do not store the digits after the floating point. Is there a reason I do not see to keep this as a double?

This is something to consider in earnest. It should be done for all digitizers coherently.

Actions #30

Updated by Volker Friese 11 months ago

Volker Friese wrote:

Pascal Raisig wrote:

One question where I am not sure how to handle it, is the type of the passed time in this case. I think it would be better to switch here to uint64 to have enough bits for the absolute time and anyhow we do not store the digits after the floating point. Is there a reason I do not see to keep this as a double?

This is something to consider in earnest. It should be done for all digitizers coherently.

The result of the analogue simulation will be a floating point. When (properly) digitized, the result will be a discrete value (multiple of the clock time). We have to think how to properly convert the latter into a global format (int or float). This needs discussion among the detector groups. Fortunately, with the current simulation (time zero is the run start, not UTC 0), we do not have precision problems as encountered with mCBM, so for the time being, we can continue going with double precision.

Actions #31

Updated by Volker Friese 11 months ago

  • Status changed from In Progress to Resolved
  • Assignee changed from Volker Friese to Felix Weiglhofer
  • % Done changed from 70 to 100

With the fixes to L1, CbmTrdDigi and CbmPsdSimpleDigitizer, everything now looks normal (on first sight). See MR 395. So I think the issue could be closed.

Actions

Also available in: Atom PDF