Skip to content
Snippets Groups Projects

Add new algorithm DecodedDataAddMCInfo

Merged Add new algorithm DecodedDataAddMCInfo
jonrob/LHCb:RICH-add-DecodedDataAddMCInfo into master
4 unresolved threads
Merged Christopher Rob Jones requested to merge jonrob/LHCb:RICH-add-DecodedDataAddMCInfo into master
4 unresolved threads

Merge request reports

Merge request pipeline #6313649 passed

Stage: test
Stage: .post

Merge request pipeline passed for 64b38667

Approval is optional

Merged by Christopher Rob JonesChristopher Rob Jones 1 year ago (Oct 9, 2023 7:47pm UTC)

Merge details

  • Changes merged into master with 480ed67e.
  • Deleted the source branch.

Pipeline #6314316 passed

Stage: test

Pipeline passed for 480ed67e on master

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
Rich/RichFutureMCUtils/src/component/DecodedDataAddMCInfo.cpp 0 → 100644
90 // PDs per module
91 for ( auto& pd : mD ) {
92 // IDs per PD
93 for ( auto& id : pd.smartIDs() ) {
94 // Do we have any MChits for this ID
95 const auto& pix_mchits = hitsPerPix[id];
96 if ( !pix_mchits.empty() ) {
97 // Find best MCHit to use. Either first labelled as single, otherwise
98 // just the first in the container
99 const auto* mch = pix_mchits.front();
100 for ( const auto* h : pix_mchits ) {
101 if ( h->isSignal() ) {
102 mch = h;
103 break;
104 }
105 }
  • Comment on lines +99 to +105
    G
    Gerhard Raven @graven ·
    Developer

    No need for an explicit loop with an if statement and a break -- just write the code like the comment describes instead (at which point the comment becomes superfluous):

    Suggested change
    99 const auto* mch = pix_mchits.front();
    100 for ( const auto* h : pix_mchits ) {
    101 if ( h->isSignal() ) {
    102 mch = h;
    103 break;
    104 }
    105 }
    99 auto i = std::find_if( pix_mchits.begin(), pix_mchits.end(),
    100 [](const auto* h) { return h && h->isSignal(); } );
    101 const auto* mch = ( i != pix_mchits.end() ? *i : pix_mchits.front() );
  • Please register or sign in to reply
  • Gerhard Raven @graven started a thread on the diff
    • Rich/RichFutureMCUtils/src/component/DecodedDataAddMCInfo.cpp 0 → 100644
    93 for ( auto& id : pd.smartIDs() ) {
    94 // Do we have any MChits for this ID
    95 const auto& pix_mchits = hitsPerPix[id];
    96 if ( !pix_mchits.empty() ) {
    97 // Find best MCHit to use. Either first labelled as single, otherwise
    98 // just the first in the container
    99 const auto* mch = pix_mchits.front();
    100 for ( const auto* h : pix_mchits ) {
    101 if ( h->isSignal() ) {
    102 mch = h;
    103 break;
    104 }
    105 }
    106 if ( m_useMCTime ) {
    107 // Set time in ID
    108 id.setTime( mch->timeOfFlight() );
    • G
      Gerhard Raven @graven ·
      Developer

      given that this is the only statement that actually has an effect, why not move the if (m_useMCTime) out of the 5 loops that surround it? No need to loop at all if m_useMCTime is false...

    • Please register or sign in to reply
  • Gerhard Raven @graven started a thread on the diff
    • Rich/RichFutureMCUtils/src/component/DecodedDataAddMCInfo.cpp 0 → 100644
    27 #include <tuple>
    28
    29 namespace Rich::Future::MC {
    30
    31 using namespace Rich::Future::DAQ;
    32
    33 /** @class DecodedDataFromMCRichHits
    34 *
    35 * Update decoded data with MC information (e.g. time)
    36 *
    37 * @author Chris Jones
    38 * @date 2016-12-07
    39 */
    40 class DecodedDataAddMCInfo final
    41 : public LHCb::Algorithm::Transformer<DecodedData( DecodedData const&, LHCb::MCRichHits const& ),
    42 Gaudi::Functional::Traits::BaseClass_t<AlgBase<>>> {
    • G
      Gerhard Raven @graven ·
      Developer

      it does not look like the inheritance of AlgBase<> is actually needed here, as there is no use of objectLocation (which should be a free-standing function) or relaseTool or acquire or messenger().

      Suggested change
      41 : public LHCb::Algorithm::Transformer<DecodedData( DecodedData const&, LHCb::MCRichHits const& ),
      42 Gaudi::Functional::Traits::BaseClass_t<AlgBase<>>> {
      41 : public LHCb::Algorithm::Transformer<DecodedData( DecodedData const&, LHCb::MCRichHits const& )> {
    • Please register or sign in to reply
  • Gerhard Raven @graven started a thread on the diff
    • Rich/RichFutureMCUtils/src/component/DecodedDataAddMCInfo.cpp 0 → 100644
    12 // Rich (Future) Kernel
    13 #include "RichFutureKernel/RichAlgBase.h"
    14
    15 // Gaudi Functional
    16 #include "LHCbAlgs/Transformer.h"
    17
    18 // Rich Utils
    19 #include "RichFutureUtils/RichDecodedData.h"
    20 #include "RichUtils/RichSmartIDSorter.h"
    21
    22 // Event Model
    23 #include "Event/MCRichHit.h"
    24
    25 #include <map>
    26 #include <memory>
    27 #include <tuple>
  • G
    Gerhard Raven @graven ·
    Developer

    It think that from first principles it would be better to not merge one's own MRs...

    • Please register or sign in to reply