Follow-up from "Allen RICH decoding"

After having a look, these are the changes that should be made:

Fix the Allen configuration to allow Rich1 and Rich2 to be decoded.
Update TransposeRawBanks to be consistent with HLT1 when it comes to the RICH banks, i.e. for Rich banks add a check on the sourceID and add the right banks to the right HLT1 source.
Update the testing algorithm to compare only smartIDs for one RICH (based on a property).
Update the test to run two testing algorithms, one for each RICH.

The Rec decoding for the Rich does Rich1 and Rich2 in one go. I guess the later algorithms take which pixels they need.

The Allen decoding does only one Rich at a time (see below for caveat in Allen-in-Moore), but "Rich1" is hardcoded in the configuration. That should be fixed.

In Allen-in-Moore, the banks of both RICHes are put into one set of raw inputs, because TransposeRawBanks only looks at the bank type. The decoding kernel then decodes everything and compares this to what the Rec decoder does, which matches. That's a good check, but running the Allen decoding like that is not really consistent with what would happen in production.

assigned to @caschbur

assigned to @sargueda

Good day! We've been working on solving this issue, but we have a couple of questions though.
1- With regards to the TransposeRawBanks update, do we want to have a different entry for Rich1 and Rich2 on the output of the function?
2- For the update on the testing algorithm, do we want to update the following test so it has a parameter that determines the RICH type we want to test and only checks for it, correct?

With regards to TransposeRawBanks a separate instance is created by the Allen-in-Moore configuration for every bank type that is requested. I'd not realised that the LHCb::RawBank::BankType is used to configure these instances and this is a significantly trickier issue than I thought.

A little bit of context for reference. In Gaudi algorithms banks are always requested from the LHCb::RawEvent by their LHCb::RawBank::BankType. In HLT1 this cannot unambiguously be used to figure out where a bank came from, and this is instead obtained in a different way. In the HLT1/Allen algorithms that deal with raw banks, the sources are identified using the BankTypes enum defined here - the name was unfortunately badly chosen. Where one entry is always one subdetector.

There are three cases where the mapping from one to the other is not one-to-one:

The Velo can produce either banks with type LHCb::RawBank::VP or banks with type LHCb::RawBank::VPRetinaCluster, but not both. In simulated samples both types may still be present.
The banks for RICH1 and RICH2 all have type LHCb::RawBank::Rich
The banks for ECal and HCal all have type LHCb::RawBank::Calo, but for older simulated samples this was LHCb::RawBank::EcalPacked and LHCb::RawBank::HcalPacked.

To fix the issue with the Rich banks that you are facing, the TransposedBanks type will have to be changed from being indexed by LHCb::RawBank::BankType to BankTypes.

This change will impact a few places:

gaudi_allen_generator.py has some logic here to change the string representation of BankTypes of the string representation of LHCb::RawBank::BankType; that can be removed.
TESProvider uses the Allen::bank_mapping to go from LHCb::RawBank::BankType to BankTypes; that can be removed.
TransposeRawBanks will need to have its m_bankTypes property changed to use the string representation of BankTypes instead of LHCb::RawBank::BankType, I'll get back to you how to do that in detail.
in operator() of TransposeRawBanks, the logic will have to be changed:
- For each BankTypes specified in m_bankTypes, loop through Allen::bank_mapping and find all the LHCb::RawBank::BankType where that BankTypes is the value. Then collect all banks with the found LHCb::RawBank::BankType from the LHCb::RawEvent in all of the RawEventLocations.
- A special case will have to be added for the RICHes. If BankTypes::Rich1 or BankTypes::Rich2 are requested, the sourceID of the banks will have to be used to figure out which of the LHCb::RawBank::Rich banks are actually needed.
- The special cases for the calorimeters and Velo have to be updated to work with the new "inverted" logic.

For the test, a property should indeed be added and the SmartIDs split by RICH to compare the relevant ones.

Hi after working on this, I have some further questions with this issue,
Working on the m_bankTypes property of TransposeRawBanks, I created a map between std::strings and LHCb::RawBank::Richs.
Additionally, I changed the m_bankTypes from a set of LHCb::RawBank::Richs to a set of std::strings. However, when I did this change I introduced an error at the initialize function which I'm not too sure how to tackle.
Do you have any advice on this front?

The cleanest way to have the property would be:

Gaudi::Property<std::set<BankTypes>> m_bankTypes {this, "BankTypes",
                                                                   {BankTypes::VP,
                                                                    BankTypes::UT,
                                                                    BankTypes::FT,
                                                                    BankTypes::MUON,
                                                                    BankTypes::ODIN,
                                                                    BankTypes::Rich1,
                                                                    BankTypes::Rich2,
                                                                    BankTypes::ECal,
                                                                    BankTypes::Plume}};

To make that possible, some functions need to be present, analogous to what is done for LHCb::RawBank::BankType here:

std::string   toString( BankTypes e ) { return bank_name( e ); }
std::ostream& toStream( BankTypes e, std::ostream& os ) { return os << std::quoted( toString( e ), '\'' ); }
std::ostream& operator<<( std::ostream& s, BankTypes e ) { return toStream( e, s ); }

StatusCode parse( BankTypes& bt, const std::string& in ) {
  auto s = std::string_view{in};
  if ( !s.empty() && s.front() == s.back() && ( s.front() == '\'' || s.front() == '\"' ) ) {
    s.remove_prefix( 1 );
    s.remove_suffix( 1 );
  }
  // Use BankSizes here because it has all he BankTypes as keys.
  auto i = std::find_if( BankSizes.begin(), BankSizes.end(), [s]( auto e ) { return s == bank_name( std::get<0>( e ) ); } );
  if ( i == BankSizes.end() ) return StatusCode::FAILURE;
  bt = i->first;
  return StatusCode::SUCCESS;
}

namespace Gaudi::Parsers {
  StatusCode parse( std::set<BankTypes>& s, const std::string& in ) {
    s.clear();
    using Gaudi::Parsers::parse;
    std::set<std::string> ss;
    return parse( ss, in ).andThen( [&]() -> StatusCode {
      try {
        std::transform( begin( ss ), end( ss ), std::inserter( s, begin( s ) ), []( const std::string& str ) {
          BankTypes t{};
          parse( t, str ).orThrow( "Bad Parse", "" );
          return t;
        } );
        return StatusCode::SUCCESS;
      } catch ( const GaudiException& e ) { return e.code(); }
    });
  }
} // namespace Gaudi::Parsers

You can find a version of TransposeRawBanks.cpp with this implemented here: https://gitlab.cern.ch/-/snippets/3025. The part where the LHCb::RawBank::BankTypes for a given BankType are obtained is commented out. I've added a mapping (std::unordered_map<BankTypes, std::unordered_set<LHCb::RawBank::BankType>>) by inverting Allen::bank_mapping from BankMapping.h. Everything up to finding the raw banks from the RawEvent should be reasonable now, but the return type is not yet correct and neither is the loop that creates the output.

The initialize function has also been fixed.

created branch 491-follow-up-from-allen-rich-decoding to address this issue

mentioned in merge request !1479 (closed)

assigned to @jruzicka

Hi @raaij! Reviewing the code changes in !1603 (merged) and Moore!3385 (merged) relating to this issue, it seems that all the pending changes have been addressed, should we close this issue now?

This has been sorted out in !1603 (merged).

closed

marked the checklist item @dcampora started a discussion: (+3 comments) as completed

Follow-up from "Allen RICH decoding"

Designs

Child items ...

Activity