Project

General

Profile

Actions

Development #2261

closed

Development #2256: Demonstrator for digi-based trigger, STS-only

Development #2257: Preparation of software infrastructure

Data containers for event and time slice digis

Added by Volker Friese 8 months ago. Updated 7 months ago.

Status:
Closed
Priority:
Normal
Target version:
Start date:
09/28/2021
Due date:
10/08/2021
% Done:

100%

Estimated time:
10.00 h
Spent time:

Description

Container classes for digis within one event (CbmRawEvent) and digis within one time slice (CbmTimeslice) are to be implemented. CbmRawEvent will be the storable output of the online processing chain; CbmTimeslice is a convenience container as input to the trigger. Both are, in the simplest representation, a collection of vectors of digis for the detector systems, plus metadata (e.g., event number, ts number, ts length).


Related issues

Blocked by Framework - Development #2259: Algo software spaceClosedVolker Friese09/28/202110/03/2021

Actions
Blocks Framework - Development #2262: Base class for triggerClosedDominik Smith09/28/202110/11/2021

Actions
Blocks Framework - Development #2263: Base class for event builderClosedDominik Smith09/28/202110/13/2021

Actions
Actions #1

Updated by Volker Friese 8 months ago

Actions #2

Updated by Volker Friese 8 months ago

Actions #3

Updated by Volker Friese 8 months ago

Actions #4

Updated by Volker Friese 8 months ago

Actions #5

Updated by Volker Friese 8 months ago

Actions #6

Updated by Volker Friese 8 months ago

I repeat here aspects which were brought up in #2262:

Jan de Cuveland wrote:

Now, about this desired interface: I understand that the question about std::any stems from the fact that you would like to only look at the timestamp of the digis, and therefore would like to have all digis in a common container - is this right? My feeling about this is that we should do without the common container. Technically, since the time stamp is stored differently in the Digis, we always need a distinction of cases. If we store the digis in a somewhat dynamically typed container, we force the compiler to include this distinction in every single access. If we keep it in different containers, however, we can either work on them independently first (without the need for distinction) and then combine, or we could as well just extract all the timestamps into a new common array as I understand it is currently done.

Then, even though the set of contributing detectors may vary, I feel that we do not need to abstract from the individual detectors. Can't we just put all the detector's Digi-level data into a structure and pass this as the input to the DigiTrigger algorithm? If a detector is foreseen, but not contributing, its data structure could be empty. At our current stage in CBM I feel that the set of detector subsystems is not that dynamic any more.

Lastly, I am still hesitant to subscribe to the notion that a detector's Digi-level data is just a vector of Digis. It may well be possible, but I can envision cases in which additional information would make sense.

All if this combined, it might lead to a structure similar to the following:

struct StsDigis {
  std::vector<StsDigi> digis;
  // ... possibly additional information ...
}

struct TimesliceDigis {
  StsDigis sts;
  TrdDigis trd;
  TofDigis tof;
  // ... more detectors ...
}

class DigiAggregationTrigger {
public:
  std::vector<double> operator()(const TimesliceDigis& ts);
};

What do you think?

Actions #7

Updated by Volker Friese 8 months ago

  • Status changed from Assigned to In Progress

Jan de Cuveland wrote:

struct TimesliceDigis {
StsDigis sts;
TrdDigis trd;
TofDigis tof;
// ... more detectors ...
}

In principle, that is what we have in CbmRawEvent (with vectors instead of detector-specific digi container classes still). This was meant to have a storable event class containing digis.

It appears that the same structure is also applicable to define the input for the digi trigger - only that the contained digis will be all from the time slice and not those attributed to an event. So I would propose something like

class DigiData (like your TimesliceDigis)

class DigiEvent : public DigiData
-- plus some event metadata

class TimesliceDigis : public DigiData
-- plus some timeslice metadata

Actions #8

Updated by Volker Friese 8 months ago

Jan de Cuveland wrote:

struct StsDigis {
  std::vector<StsDigi> digis;
  // ... possibly additional information ...
}

I support this view, but I wanted, in the first stage, to avoid lengthy discussions on how the detector-specific container shall look like. For instance, what are the metadata, one vector or several, etc. However, just defining the struct/class with nothing but a single vector and leaving future enhancements to the detector groups is probably a good approach since one can go ahead with the DigiData class independently from that.

To the naming: "StsDigis" is too close, in my view, to StsDigi and might lead to confusion. May be "StsDigiData"?

Actions #9

Updated by Volker Friese 8 months ago

I wonder if we can go, for the detector-level digi container, with the simple struct you proposed (where the vector is a public member). Of course this is easiest, but also unsafe. The class will be used, as far as I can see now, both by reconstruction (hit finder) as well as by digi-based event building. Allowing the user code to change the input data is not what we usually do. The alternative would be to have the vector as private member and wrap the basic functionality of std::vector.

From the consumer side, I think it is enough to provide const access to the vector. From the producer side (unpacker and event builder), both adding a vector<Digi> and adding single digis would be needed.

Actions #10

Updated by Jan de Cuveland 8 months ago

To the naming: "StsDigis" is too close, in my view, to StsDigi and might lead to confusion. May be "StsDigiData"?

D'accord.

I wonder if we can go, for the detector-level digi container, with the simple struct you proposed (where the vector is a public member). Of course this is easiest, but also unsafe. Allowing the user code to change the input data is not what we usually do.

To my understanding, input data should be passed as const, e.g. a const ref, so that it cannot be modified. This should be independent of whether we use public accessor methods or data members.

A consequence, however, of this approach is that we expose the internal structure of the data container. I don't call it an object here, because I think not all data structures need to be considered objects in the sense that you communicate with them rather than use/work on them.

Actions #11

Updated by Volker Friese 7 months ago

  • Assignee changed from Volker Friese to Jan de Cuveland
  • % Done changed from 0 to 40

Taking into account the discussion above, I have implemented first versions for digi containers on detector, event and timeslice level. At the moment, only STS is included, until the other digi classes will be disrooted as CbmStsDigi was. Replacing this simple wrapper by an actual detector-specific implementation should be easy, once needed.

At detector level, I use a simple wrapper around std::vector (templated to save coding) with the expected access patterns (filling from unpackers: add vector<Digis>, filling from event builder: add single digi, reading: indexed access to single digis).

As said above, the requirements for timeslice and event digi containers are mostly identical save some meta data, so there is a base class CbmDigiData, where CbmDigiEvent and CbmDigiTimeslice derive from. If any more time slice meta data are needed, please let me know.

Please have a look at the basic structure (and implementation), to be found in the branch onlinedata of https://git.cbm.gsi.de/v.friese/cbmroot (commit https://git.cbm.gsi.de/v.friese/cbmroot/-/commit/eb3fbe90a4673adc9cc4893cd0937206878513ce). Meanwhile, I will go ahead using and testing these interfaces for the API of digi trigger and event builder.

Actions #12

Updated by Jan de Cuveland 7 months ago

  • Assignee changed from Jan de Cuveland to Volker Friese

I'm sorry I didn't comment earlier here. I do have "some" remarks/considerations. ;-)

core/data/base/CbmDataVector.tpl

From what I found online, my impression is that wrapping STL containers without adding substantial own functionality is generally discouraged. The main arguments are that it requires additional maintenance and code complexity to replicate the full functionality, or otherwise missing functionality, which may result in reduced performance. In addition, the additional template layer produces more complex warning and error messages.

In this case of CbmDataVector, e.g., it seems to me that it is impossible to generate the entries in place without copying through emplace_back(). Move semantics do not help here as PODs can only be copied. Also, an algorithm operating on this data structure is restricted to a single foreseen access pattering using the GetData() function, which performs a range-check on every access. More efficient access patterns like a range-based for loop are excluded.

Generally, most of our algorithms will use specific inputs and outputs (instead of in/out parameters). So it does not really /operate on/ a data structure and I don't see the need to restrict it to specific access patterns in using its input or generating its output.

That's why I have the feeling we will be better off if we drop the CbmDataVector and stick with the plain std::vector here. I have a few more detailed comments on CbmDataVector.tpl, but I think it makes sense to discuss this in detail only if you decide to keep it.

More generally speaking, should the input/output interface to algorithms consist of complex objects or rather mostly functionless data containers? If we allow complex objects, how can we make sure that there is no functionality included that dilutes the intended separation of data and algorithm? On the other hand, for pure data containers, there is not much gain in providing accessor methods. I understand that data encapsulation is a key paradigm of object-oriented programming. But I think that in the interface to the algorithms, which is almost like an external interface, a more functional approach may yield better readability and performance.

core/data/base/CbmDigiData.*

Can CbmDigiData.h be compiled without ROOT if it includes CbmDefs.h? I suspect that CbmDefs.h in its current form pulls in the ROOT requirements through RtypesCore.h.

Here I am not sure about the inheritance vs. composition question. Is there an algorithm that would accept dynamically either a CbmDigiEvent or a CbmDigiTimeslice? If not, we could also think about including CbmDigiData in different aggregate structures instead of inheriting from it, removing the need for polymorphism.

I think the virtual destructor is not needed here because the class does not manually allocate memory.

As above, I see some issues, especially regarding performance, with wrapping access to the STL types.

Also in the Add() functions, do we need the ECbmModuleId here? It may be redundant with the type of Digi as in: void Add(StsDigi data) { fSts.AddData(data); }

core/data/global/CbmDigiEvent.*

If we go for composition instead of inheritance, this could be a struct containing both a CbmDigiData and a DigiEventDescriptor (or similar) member.

In the DigiEventDescriptor, how would the event number be generated? Is this redundant to the position in the surrounding container?

Is #include <type_traits> really needed here?

core/data/global/CbmDigiTimeslice.*

The CbmDigiTimeslice could well contain some more metadata, but if we put this in a DigiTimesliceDescriptor or TimesliceMetadata struct, we can always add more members later on.

And finally, a more general question:

Should this new code be in the new /algo directory?

Conclusion

So, wrapping this all up, I think the primary question is to what extent we want to apply the data hiding principles (through accessors and mutators) in these data structures.

It seems to me that for the Digi containers, performance is especially relevant. If we want to allow the algorithms to work at full performance, we need to provide access to the data structures at a rather low level. That means either direct access in a struct (public data member) or a low-level accessor function could, e.g, directly return a reference to the stl vector for the algorithm to work on.

Also, we create an interface to algorithms, which is fundamentally based on data structures, not methods or objects.

After some research, I did not find matching guidelines for this particular situation. There are many arguments for accessors and mutators in C++, but most of them seem to rather contradict our goals here.

  1. "Data hiding is a process of combining data and functions into a single unit." - Yes, but we specifically want to disentangle data structures and algorithms, not combine them.
  2. "Data hiding ensures exclusive data access to class members and protects object integrity by preventing unintended or intended changes." - Yes, but we pass these containers as input/output between the algorithms, and we specifically don't want to mutate them while continuously ensuring object integrity.
  3. "Data hiding allows you to change the underlying data structure without the user noticing" - Yes, but in the algorithm interface the input/output data structure is specifically part of the contract. There is hardly anything we could change in its internal structure without requiring a corresponding change in the algorithm or a causing performance penalty.

I feel that OOP is certainly useful, but not the "end all" programming paradigm that separates good code from bad. In this case, I would imagine that using simple structures instead of objects would both solve potential performance issues and reduce the size of the code.

Actions #13

Updated by Volker Friese 7 months ago

Thanks for the quick feedback! I appreciate and share most of your arguments. Of course, the main question is whether we want data encapsulation or go with simple structures with public data members. Obviously, the latter leads to leaner code.

core/data/base/CbmDataVector.tpl

From what I found online, my impression is that wrapping STL containers without adding substantial own functionality is generally discouraged. The main arguments are that it requires additional maintenance and code complexity to replicate the full functionality, or otherwise missing functionality, which may result in reduced performance. In addition, the additional template layer produces more complex warning and error messages.

I agree that the price for data encapsulation in this case - additional code and restricted access patterns - is particularly high, and I am happy to remove it. I saw it as a placeholder for a more detailed implementation anyhow.

More generally speaking, should the input/output interface to algorithms consist of complex objects or rather mostly functionless data containers?

Do you consider std::vector a complex object or a functionless data container?

If we allow complex objects, how can we make sure that there is no functionality included that dilutes the intended separation of data and algorithm?

Well, that's in our own hands, is it not? Who else would be programming the code?

core/data/base/CbmDigiData

Can CbmDigiData.h be compiled without ROOT if it includes CbmDefs.h? I suspect that CbmDefs.h in its current form pulls in the ROOT requirements through RtypesCore.h.

CbmDefs.h is needed just because of the use of the enum class ECbmModuleId. We might go without that one, but CbmDefs itself is ROOT-free and does not include RtypesCore.h nor any other ROOT class.

Here I am not sure about the inheritance vs. composition question. Is there an algorithm that would accept dynamically either a CbmDigiEvent or a CbmDigiTimeslice?

The algorithms (with the exception of the digi-based trigger) will not see DigiTimeslice or DigiEvent, but a vector of digis extracted from that, which happens on framework (task/device) level. Extraction of, say, the StsDigiData will be the same for DigiEvent and DigiTimeslice. Since the reco algorithms should run both on event (online with digi trigger, offline) and timeslice data (online without digi trigger), the task/device code can be unified having a base class DigiData.

If not, we could also think about including CbmDigiData in different aggregate structures instead of inheriting from it, removing the need for polymorphism.

There is no polymorphism here, no virtual methods, not even overloading, just plain inheritance to share common functionality. From the point of view of coding, I found inheritance leaner than aggregate structures, but that might in the end be a matter of taste. I think that on container level, there are no strong (performance) arguments against inheritance.

I think the virtual destructor is not needed here because the class does not manually allocate memory.

In my understanding, the argument is not correct. It is not the question whether the base class dynamically allocates memory, but whether the derived classes do, such that a call to the destructor from a pointer to the base class will leave a resource leak if the destructor is not virtual.
In our case here, both (base and derived) destructors are empty, so no virtuality is needed. Since the destructor is the only virtual method, we could spare the vtable entry. However, in general I do not think it a good strategy to restrict possible derived classes when implementing the base class.

Also in the Add() functions, do we need the ECbmModuleId here? It may be redundant with the type of Digi as in: void Add(StsDigi data) { fSts.AddData(data); }

Right, when you code the Add method explicitly for all detectors, you do not need the enum, only when you want to use a function template. Explicit coding might be better also to avoid the switch within the method.

core/data/global/CbmDigiEvent

In the DigiEventDescriptor, how would the event number be generated? Is this redundant to the position in the surrounding container?

We have not decided on that yet (unique event identifier or w.r.t. current time slice). I think in offline analysis, the basic entity is the event, and time slices do not play much of a role, so a unique event identifier (running number within the data taking run) is probably the appropriate choice. For reference, the time slice index should then be added. We have to think about the time slice overlaps, though.

In the ROOT file, the (current) structure is that a time slice is a tree entry, and the events in the time slice are leaves within. So, a reference to the time slice with in the event struct is not strictly needed. But we should foresee also other storage models, and an additional TS index in the event struct probably does not hurt.

Is #include <type_traits> really needed here?

No, it is not. Just my IDE analyser complained (for a while), but it compiles without.

core/data/global/CbmDigiTimeslice.

The CbmDigiTimeslice could well contain some more metadata, but if we put this in a DigiTimesliceDescriptor or TimesliceMetadata struct, we can always add more members later on.

No objection. However, I do not clearly see the difference / advantage between changing the descriptor and changing the DigiTimeslice class itself. Unless, of course, we could use the existing flesnet TimesliceDescriptor.

And finally, a more general question:

Should this new code be in the new /algo directory?

That is what I thought - also to organisationally separate the "online-capable" / ROOT-free code from the rest. However, for the case of CbmStsDigi, Florian did not move it from its current location (see #2260). We should discuss this with him, and I think the movement can also be done later (but should not be forgotten). The strategy to compile into a library libCbmOnlineData (w/o ClassDef) in addition to the ROOT-dependent library libCbmData is followed for the new classes, too, so that is independent from where the actual code is.

Conclusion

So, wrapping this all up, I think the primary question is to what extent we want to apply the data hiding principles (through accessors and mutators) in these data structures.

"Data hiding ensures exclusive data access to class members and protects object integrity by preventing unintended or intended changes." - Yes, but we pass these containers as input/output between the algorithms, and we specifically don't want to mutate them while continuously ensuring object integrity.

I agree with CbmDigiTimeslice, which is transient anyway. CbmDigiEvent as storage entity, on the other side, is the interface into the offline world. Mainly because of that I felt uneasy to go unprotected against unintended changes, affecting then all other "users".

"Data hiding allows you to change the underlying data structure without the user noticing" - Yes, but in the algorithm interface the input/output data structure is specifically part of the contract. There is hardly anything we could change in its internal structure without requiring a corresponding change in the algorithm or a causing performance penalty.

Normally, not the internal data representation, but the access API is part of the contract. Just imagine the hypothetical case that a detector group decides to use another underlying container instead of std::vector.

Bottom line

I will prepare another version with simple structs replacing CbmDigiData as you propose. I would like to keep the inheritance CbmDigiData -> CbmDigiTimeslice, CbmDigiEvent for the reasons above (but also in view that CbmDigiEvent should be storable in ROOT files), but give public access to the data members.

Actions #14

Updated by Volker Friese 7 months ago

  • Assignee changed from Volker Friese to Jan de Cuveland
  • % Done changed from 40 to 60

OK, here comes another version:
https://git.cbm.gsi.de/v.friese/cbmroot/-/tree/digidata
https://git.cbm.gsi.de/v.friese/cbmroot/-/commit/0e056fb4cc77cfdaa691596e16f9244c29056ec9

All structures are implemented as aggregates. I withdraw my argument for inheritance from DigiData. If this really turns out to be advantageous, it can be introduced when the need shows up. I note, though, that with the C++17 standard, aggregates can have non-virtual base classes, though.

For CbmDigiTimeslice, I use fles::TimesliceDescriptor as member, although I am not sure whether it is wise to introduce such a dependence to the data library.

I have implemented the usage of CbmDigiEvent in the ideal event builder as a test, and it works nicely. The data are browsable in TBrowser.

The location of the source code and the usage of namespaces still has to be discussed, which should not prevent us here from proceeding.

If you agree with the basic layout, I can issue a merge request, where the code still can be polished.

Actions #15

Updated by Pierre-Alain Loizeau 7 months ago

For CbmDigiTimeslice, I use fles::TimesliceDescriptor as member, although I am not sure whether it is wise to introduce such a dependence to the data library.

If we do not want an external dependence, wouldn't TimesliceMetadata do the job if we removed the TObject inheritance? (which I probably added only to use TClonesArrays)

This class is for now probably too lax on field sizes and missing some fields, but my idea was anyway to first fill it with info and then optimize the storage, as we have only one of them per Timeslice. For now only things needed for mCBM where added whenever somebody asked for them.

The advantage of having an independent descriptor is that at some point we can transfer only this one for access to the original Flags/Info, without needing access to the hit data level or copying each field independently, for example when preparing a HitTimeslice or TrackTimeslice (if we ever need them).

Actions #16

Updated by Jan de Cuveland 7 months ago

Thanks for the explanations and clarifications. I follow your arguments.

Do you consider std::vector a complex object or a functionless data container?

Probably "functionless" was not the right term to use here. An STL object certainly contains member functions, but in this sense I am still calling it "functionless". Maybe I can illustrate what I meant in an example: Suppose the mapping of something (say, physical to logical channels) was a linear function, m*x+b. A parameter container passed to the unpacker can then be expected to contain the two values m and b. It could then either expose these two values directly, or implement a function PhysicalToLogical(x), which contains the, in this case trivial, computation. However, I think we should rather avoid the latter solution here, as it invites shifting some of the computations from the unpacking algorithm to the parameter container. This can complicate the analysis and optimization of the algorithm. If we start with that, I don't see a clear dividing line. In a hypothetical extreme case, we would arrive at a parameter class that contains an ApplyToRawData(ts) method that fully contains and replaces the unpacker algorithm. So, "functionless" in this sense basically meant: not working with the actual data. I find it hard to specify that more formally.

I think the virtual destructor is not needed here because the class does not manually allocate memory.

In my understanding, the argument is not correct. It is not the question whether the base class dynamically allocates memory, but whether the derived classes do, such that a call to the destructor from a pointer to the base class will leave a resource leak if the destructor is not virtual.

Yes, you are right. If we want to allow for a derived class with a non-empty destructor, the virtual destructor here is definitely recommended. My thinking was: If we add such a derived class at some point, we can as well make the destructor virtual in the base class at that point. Also, to my understanding, the destruction of in-class members such as smart pointers works correctly even without a virtual destructor. Since the advent of smart pointers, I very rarely have a need for non-empty destructors.

In the DigiEventDescriptor, how would the event number be generated?

I think in offline analysis, the basic entity is the event, and time slices do not play much of a role, so a unique event identifier (running number within the data taking run) is probably the appropriate choice.

To my understanding, we will create the event data structures as part of the online processing. And we aim to process timeslices independently in parallel. I don't think that it will be feasible to communicate online a global running number within the data taking run between all processing nodes for each event.

Thus, I think the event number has to be relative to the timeslice. The unique event identifier could be the tuple of timeslice number and event number, if we want a simple continuous numbering scheme. Or we could use the event's global real time in nanoseconds as an identifier, which should also be globally unique.

For CbmDigiTimeslice, I use fles::TimesliceDescriptor as member, although I am not sure whether it is wise to introduce such a dependence to the data library.

I am also not sure about this dependency. In the future, it might be useful to change this to a more reconstruction-oriented version of the descriptor. However, as this should be a fairly straightforward change conceptually, we can always proceed now and refine it later.

If we do not want an external dependence, wouldn't TimesliceMetadata do the job if we removed the TObject inheritance?

I believe that the correct handling of microslice and timeslice overlap in our whole system requires some more thought. I feel that what we currently do in flesnet is already too simple, and I am quite sure that the present support in TimesliceMetadata is not sufficient. What I envision here is that we could have basically three numbers for each timeslice: (1) a local time "zero", i.e. an offset to UTC that all contained digi timestamps are relative to, (2) a "start time", which marks the beginning of the authoritative interval, and (3) an "end time", which marks the end of said interval. However, I would prefer to discuss this idea with a group of people before introducing it as a new data structure.

Actions #17

Updated by Volker Friese 7 months ago

  • % Done changed from 60 to 80
Actions #19

Updated by Volker Friese 7 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 90 to 100
Actions

Also available in: Atom PDF