CbmRoot » Simulation

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.

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.

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.

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().

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.

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

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

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.

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)?

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).

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?

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

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.

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) 

//_________________________________________________________________________________
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

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?

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.

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.

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.