Understanding the structure of TRD2D.

requested review from @d.smith

assigned to @d.smith

resolved all threads

added 1 commit

7b71acbb - Shortened CbmTrdUnpackFaspAlgo::pushDigis().

@a.bercuci I have a question about this object:

 std::map<uint16_t, std::array<std::vector<CbmTrdDigi>, NFASPMOD* NFASPCH>> fDigiBuffer = {
    {{}}};  ///> Buffered digi for each pad in CROB component

The way I understand it, this is a container for temporary storage of digis during the execution of the unpacker. It is needed, because messages which arrive later in the stream can modify the properties of digi objects which have already been instantiated (e.g. by setting the "R" and "T" components).

This object has three indices. The following is an example of this object being used in the code:

fDigiBuffer[fCrob][pad].emplace_back(pad, lchT, lchR, lTime);

This suggests the indices are (crob index, pad index, digi index).

Now my question: The steering object which manages unpacking, always calls the function Unpack() of CbmRecoUnpackAlgo.tmpl with a fixed timeslice component index. This function calls CbmTrdUnpackFaspAlgo::unpack() for each microslice, and subsequently CbmTrdUnpackFaspAlgo::FinalizeComponent() when the microslice loop has completed. The way I see it, fCrob (which is simply the component index), remains unchanged throughout this entire process. And when FinalizeComponent() is called, the digi buffer is cleared for each pad index, so there is no way digis can be carried over to the next compoment.

So my question is: Do we need the field fCrob at all? From what I can see, the only place it is actually used in the code is to produce a warning message for incomplete digis, i.e. this block in CbmTrdUnpackFaspAlgo::FinalizeComponent():

  if (nIncomplete > 2) {
      LOG(warn) << fName << "FinalizeComponent(" << fCrob << ") skip " << nIncomplete << " incomplete digi at pad "
                << ipad << ".\n";
    }

If this message is needed, then fCrob probably needs to be kept, but the field fDigiBuffer can surely be simplified by dropping the first index, i.e. something like:

 std::array<std::vector<CbmTrdDigi>, NFASPMOD* NFASPCH> fDigiBuffer = {{}};  ///> Buffered digi for each pad in CROB component

added 1 commit

ac72545d - CbmTrdUnpackFaspAlgo: Reintroduced the verbose option in unpack().

Compare with previous version

@a.bercuci Also regarding my comment above: There exists a function CbmTrdUnpackFaspAlgo::ResetTimeslice() which is called from CbmTrdUnpackFaspConfig::reset(), which loops through all elements of fDigiBuffer and looks for unprocessed digis, and then outputs an error message stating their number.

At least with the current task-based setup, it seems there is no way any unprocessed digis can ever be found here, as CbmTrdUnpackFaspAlgo::FinalizeComponent() will have already cleared the corresponding vector.

Do you know any scenario where the functionality of CbmTrdUnpackFaspAlgo::ResetTimeslice() is required?

On edit:

I just looked at the FairMQ device CbmDeviceUnpack, and it appears that also at the FairMQ level Unpack() of CbmRecoUnpackAlgo.tmpl is used, so FinalizeComponent() is implicitly called. It appears to me that the content of CbmTrdUnpackFaspAlgo::ResetTimeslice() can safely be removed. Probably it was used in some older version of CbmRoot and is no longer needed.

added 1 commit

fd6f438d - CbmTrdUnpackFaspAlgo: Simplified digi buffer.

Compare with previous version

added 1 commit

efdebe92 - CbmTrdUnpackFaspAlgo: Reintroduced ResetTimeslice().

Compare with previous version

resolved all threads

@a.bercuci A question about the following code block, which is found in CbmTrdUnpackFaspAlgo::unpack()

// get MOD_id and CROB id from the equipment
  const uint16_t eq_id = msdesc.eq_id;
  bool mapped          = false;
  for (auto mod_id : fModuleId) {
    for (crob_id = 0; crob_id < NCROBMOD; crob_id++) {
      if (((*fCrobMap)[mod_id])[crob_id] == eq_id) break;
    }
    if (crob_id == NCROBMOD) continue;

    // found module-cri pair
    // buffer module configuration
    if (fMod == 0xffff || fMod != mod_id) {
      fMod = mod_id;
      if (!init()) {
        LOG(error) << GetName() << "::unpack - init mod_id=" << mod_id << " failed.";
        return false;
      }
    }
    mapped = true;
    break;
  }

As the comment states, it obtains a (mod_id, crob_id) pair from the equipment id.

From what I understand, 0xffff is the initial value of fMod given by the class constructor, but not an actual value that fCrobMap will contain. In this case, the first part of if (fMod == 0xffff || fMod != mod_id) is redundant, since it can never happen that fMod gets set back to 0xffff once it was something else.

Furthermore, the init() function in this class is defined as init() { return kTRUE; } so it doesn't actually do anything, and the LOG(error) statement can never be triggered.

One could in principle envision a use-case where we would define init() to carry out some tests. But this potentially breaks the FairTask model. In the base class CbmRecoUnpackAlgo.tmpl init() is defined as a virtual function, to be implemented by derived classes, which should be run once at the beginning of program execution. Here we are calling it from within unpack() however, which is repeatedly called during the execution stage. So if such a check were to be implemented, it should probably get its own function and not use init().

I would propose at a minimum to replace the entire block by:

// get MOD_id and CROB id from the equipment
  const uint16_t eq_id = msdesc.eq_id;
  bool mapped          = false;
  for (auto mod_id : fModuleId) {
    for (crob_id = 0; crob_id < NCROBMOD; crob_id++) {
      if (((*fCrobMap)[mod_id])[crob_id] == eq_id) break;
    }
    if (crob_id == NCROBMOD) continue;

    // found module-cri pair
    // buffer module configuration
    fMod = mod_id;
    mapped = true;
    break;
  }

This change should be safe.

Furthermore, what we are actually doing here, is inverting the one-to-one map M: (mod_id,crob_id) -> (eq_id). This map M is supplied externally through calling CbmTrdUnpackFaspAlgo::SetCrobMapping(), but what we actually need at runtime is M^-1: (eq_id) -> (mod_id,crob_id).

The inversion of M is re-computed in each call of unpack(). Not sure how much of a performance hit this ultimately is, but it certainly something we could avoid.

What I would propose is to compute the inversion one in CbmTrdUnpackFaspAlgo::SetCrobMapping(), and then store M^-1 instead of M. @a.bercuci if you agree, I will try to implement this.

added 1 commit

5ad7b054 - CbmTrdUnpackFaspAlgo: Simplified mod/crob mapping in unpack().

Compare with previous version

added 1 commit

5a36f814 - CbmTrdUnpackFaspAlgo: Switched to storage of inverted instead of direct crob map.

Compare with previous version

added 32 commits

5a36f814...9c17ea1f - 24 commits from branch computing:master
c4171f05 - Cleared out CbmTrdUnpackFaspAlgo.
c2eec86e - Shortened CbmTrdUnpackFaspAlgo::pushDigis().
c464ea65 - CbmTrdUnpackFaspAlgo: Reintroduced the verbose option in unpack().
fa4e00a4 - CbmTrdUnpackFaspAlgo: Simplified digi buffer.
3ca43a4c - CbmTrdUnpackFaspAlgo: Reintroduced ResetTimeslice().
aed08062 - CbmTrdUnpackFaspAlgo: Simplified mod/crob mapping in unpack().
78dad076 - CbmTrdUnpackFaspAlgo: Switched to storage of inverted instead of direct crob map.
c0cbee28 - CbmTrdUnpackFaspAlgo: Made fMod a local variable and other small changes.

Compare with previous version

@a.bercuci Ok, I just pushed my latest minor changes. So far, the code has only been modified in cosmetic ways. This code behaves exactly as the old one, so all of the "verbose" messages and commented-out code blocks remain in place. If you like, you can merge this version, or not, it is up to you. I will remove the Draft flag in any case.

I will start to break stuff now, which I will do in a different branch. I think I have mostly understood what needs to be done for a cbm::algo implementation.

marked this merge request as ready

Dear @a.bercuci, @p.kaehler,

you have been identified as code owner of at least one file which was changed with this merge request.

Please check the changes and approve them or request changes.

Understanding the structure of TRD2D.

Merge request reports

Activity