Skip to content
Snippets Groups Projects

Draft: DevelopmentTagger enum and string conversion

Open Draft: DevelopmentTagger enum and string conversion
tagging_ift_v2 into run2-patches
2 unresolved threads
Open Vukan Jevtic requested to merge tagging_ift_v2 into run2-patches
2 unresolved threads

Adds enum for the old DevelopmentTagger that is reenabled for future FT WG Run2 Tuple Production.

Note: TaggerTypeToString and TaggerTypeToType are only called in BTaggingTool.cpp in Phys in few places, therefore impact is small.

Modernization was needed because previous static map implementation behaved strangely with lb-checkout LHCb/branch_with_changes Event: Newly implemented tagger types in the string-type mapping were unavailable in Phys/FlavourTagging and only the old ones were found. At the same time, all other methods implemented in Tagger.h behaved as expected. This implementation ties the static map to the Tagger class, which seems to solve this issue.

Edited by Vukan Jevtic

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Gerhard Raven @graven started a thread on the diff
    • Event/PhysEvent/include/Event/Tagger.h
    117 120 static LHCb::Tagger::TaggerType TaggerTypeToType( const std::string& aName );
    118 121
    119 122 /// conversion to string for enum type TaggerType
    120 static const std::string& TaggerTypeToString( int aEnum );
    123 static const std::string& TaggerTypeToString( Tagger::TaggerType aEnum );
    • G
      Gerhard Raven @graven ·
      Developer

      The fact that the argument is a TaggerType makes the name somewhat redundant. Better to just call it

      Suggested change
      111 static const std::string& TaggerTypeToString( Tagger::TaggerType aEnum );
      111 static const std::string& toString( Tagger::TaggerType aEnum );

      and be consistent with lots of other enums.

    • Vukan Jevtic
      Vukan Jevtic @vjevtic ·
      Author Developer

      I fully agree with the reasoning, but if I did this, this would require changing all single tagger definitions and be inconsistent with the code on master which uses the verbose naming style. Would you still like me to apply this change with a follow-up MR to master? (Also I would suggest to replace the growing switch statement on master with my implementation to keep things simpler.)

    • G
      Gerhard Raven @graven ·
      Developer

      let's just do this on master, and keep the changes to run2-patches as small as possible (and where necessary, and applicable, 'backported' from master to avoid diverging)

    • Please register or sign in to reply
    • G
      Gerhard Raven @graven ·
      Developer

      Modernization was needed because previous static map implementation behaved strangely with lb-checkout LHCb/branch_with_changes Event: Newly implemented tagger types in the string-type mapping were unavailable in Phys/FlavourTagging and only the old ones were found. At the same time, all other methods implemented in Tagger.h behaved as expected. This implementation ties the static map to the Tagger class, which seems to solve this issue.

      The above is not a surprise: when changing something in a public header in eg. LHCb, all code which includes that header has to be (re)compiled consistently, as any code that is not compiled with this version of the header will have been compiled with the old version of the header. And since the string-type mapping was in the header, there must have been code that should have been recompiled, but wasn't. This is the what makes lb-dev a double-edged knife: it uses less space / time because it compiles much less code, but sometimes, it doesn't compile enough code.

    • Vukan Jevtic
      Vukan Jevtic @vjevtic ·
      Author Developer

      This should be solved now that the map is declared in the source file, right?

    • Please register or sign in to reply
  • Vukan Jevtic added 1 commit

    added 1 commit

    • 9056068a - Separate implementation of class Tagger.

    Compare with previous version

  • Vukan Jevtic added 1 commit

    added 1 commit

    Compare with previous version

  • Vukan Jevtic added 1 commit

    added 1 commit

    Compare with previous version

  • Vukan Jevtic added 1 commit

    added 1 commit

    • dce8b11e - default ctr,dtr,copy constructor

    Compare with previous version

  • Vukan Jevtic added 1 commit

    added 1 commit

    Compare with previous version

    • Please register or sign in to reply