New and faster decoding versions

Requires MR!10 from SIMCOND.

Introduced new raw data format (v4 and v5). The new data format has no local headers for each SiPM anymore. This improves the decoding speed a lot. Also new is the partitioning (5 banks per quadrant instead of 1) and a global truncation header.

For backwards compatibility, the decoding can still decode the old data formats (v2 and v3). The difference between v4 and v5 is that in v5 the adjacent clusters (that are split by the FE) are merged back and that very wide clusters are recovered into many adjacent clusters. These extra operations are not done in v4, which makes it a little bit faster at the costs of a reduction in tracking performance (possible these extra operations can be done on the TELL40 in the future). The encoding works only for the v5 format. To run the v4 decoding on v5 data, a flag is introduced in FTRawBankDecoder (ForceVersion4).

At same time, the container format for the FTLiteClusters is changed from FastClusterContainer to MultiIndexedContainer.

Closes LHCBSCIFI-101.

@sesen @lohenry @rquaglia

In case you know you will need (upto) 48 values, either use std::array<int,48> (always 48) or boost::static_vector<int,48> (in case you sometimes need less than 48), both of which will avoid allocating memory on the heap.

Thanks! It's always 48 as far as I understood (3 stations, 4 layers, 4 quadrants). However, we want to fill it in order (using push_back then), so the boost solution looks much better.

Isn't there already header which defines eg NStations, NLayers and NQuadrants (to be 3,4 and 4 respectively)? In that case, I'd write this as std::array<int,NStations*NLayers*NQuadrants> -- and if not, I'd introduce those three constants first ;-)

(note: these three constants should be qualified constexpr so that their product can be evaluated at compile-time)

OK I have implemented that! There is probably such a header but right now I have put that into FTRawBankParams.

Putting if statements in a loop will not make it go faster. Please consider splitting things (duplicating!) into two loops: one which does the work, and one which just does the printout -- i.e. something like:

   const auto last  = bank->end<short int>();
   if (msgLevel(MSG::VERBOSE)) {
      auto first = bank->begin<short int>();
      while (first!=last) {
        // generate just the output
      }
   }
   auto first = bank->begin<short int>();
   while (first!=last) {
       // do the decoding without any verbose() output
   }
}

In case that generates to much 'duplicate code', then consider doing something along the lines of:

enum class Verbose { True, False };

FTLiteClusters
FTNewRawBankDecoder::operator()(const LHCb::RawEvent& rawEvent) const {
   return msgLevel(MSG::VERBOSE) ? decode<Verbose::True>(rawEvent) : decode<Verbose::False>(rawEvent);
}

template <Verbose verbose>
FTLiteClusters
FTNewRawBankDecoder::decode( const LHCb::RawEvent& rawEvent) const {
{
   // take current code in operator(), and replace 'msgLevel(MSG::VERBOSE)' with 'verbose==Verbose::True'
}

This will then generate two versions of decode, one 'slow' one with printout, and a 'fast' one without any printout, and do the check just once, and dispatch the relevant version.

That's probably the way to go (template coding rather than code duplication), bthanks a lot for the example! I will first go to a compiling/working version of the code though.

Do you really need a separate tool to determine the channelID?

One reason to have such a tool is if there are multiple implementation which you want to be able to interchange, and thus you need a well-defined interface (IFTReadoutTool in this case). But is there really more than one implementation necessary?

Another reason would be if there would be another place where the same functionality is required, i.e. if the tool would be shared between algorithms. So the question then is, is that the case?

If either of the two questions is yes, then there still remains the question on whether an interface which accepts a single unsigned, and returns an FTChannelID is optimal. For example, is it possible to return (a reference to) a (concrete) map (lookup table, ...) from modulesipm to (mat,sipm) which can be retrieved outside of the loop (and thus avoid a virtual function call inside the loop)

Leaving the two first questions to later, what do you have in mind? Another property of FTRawBankDecoder that point to an xml (probably linked to the DDDB) that makes the mapping FTChannelID -> (module/sipm/channelID), leaving only station/layer/quarter/mat to the logical operations we are performing?

What I have in mind is just to move the code currently in FTReadoutTool into this algorithm -- and if that takes another property here, then yes, that should be added. So what are the downsides of just doing that?

why must an instance of IFTReadoutTool be explicitly initialized? Can the implementation not figure this out itself?

I am unfamiliar with the Interface pattern, and its interplay with Gaudi. What would be the piece of code/framework that takes care of initialisation by itself? Thanks a lot for your helpful comments.

Looking at the contents of FTReadoutTool, I wonder why it is a separate tool, and not just substituted 'in situ' in this algorithm. What triggered me is the need for an initialize() method in the tool. That suggest that the code must be tied into the Gaudi state machine, and at that point, you have an algorithm, not a tool. Hence the questions below, which are aimed at figuring out what is really needed, and whether a tool is the right thing to implement this.

I would avoid adjectives like 'New' instead class names, as it will look weird in a few years time (and what will you call the next version? FtNewerRawBankDecoder?)

Instead, I'd put it in a namespace with a version number. And given that I think it would be good to put subsystem code in a dedicated namespace which is close to the package name, why not something like

namespace LHCb { namespace FT { namespace DAQ {  

   namespace v2 {

   class RawBankDecoder : ...

   }

 } } }

Ideally, all subsystems would adopt such a common namespace pattern... (and eg. Rich::DAQ does already exist ;-)

I did not know about the namespace pattern, that seems neat! Concerning the NewRawBankDecoder, this is temporary (and I mean, really temporary) because we don't know which version we want to use and I wanted to start working in parallel. As I still try to master the intricacies of GIT I did not want to use two branches.

You can write firstChannel here

mentioned in merge request Rec!1072 (merged)

mentioned in merge request Boole!153 (merged)

changed the description

added 1 commit

• fc44900a - Simplify estimate for number of clusters from raw bank

Compare with previous version

added 2 commits

• 9e9058a3 - Size flag in cluster words is now also set for first cluster
• 6042d5e2 - Move Lambda functions out of loop

Compare with previous version

added 1 commit

• 54bdfd8e - Split output from outer Tell40s in two banks and add 32b truncation header

Compare with previous version

changed the description

• [2018-05-31 00:13] Validation started with lhcb-gaudi-head#1883
• [2018-05-31 11:06] Validation started with lhcb-gaudi-head#1884

added 1 commit

• cee5f374 - Fix typo in MultiIndexHitContainer

Compare with previous version