Skip to content
Snippets Groups Projects

Fixes to DD4hep alignment code

Merged Fixes to DD4hep alignment code
sponce_DD4hepAlignSupport into master
Merged Sebastien Ponce requested to merge sponce_DD4hepAlignSupport into master

The 2 main directions being :

  • proper handling of units, ROOT and LHCb ones being different
  • proper cleanup of cached data when changing geometry

Goes along with LHCb!2998 (merged), Rec!239 (merged), Alignment!168 (merged) and AlignmentOnline!49 (closed).

Edited by Miroslav Saur

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
Core/tests/src/test_DetectorDataService.cpp
150 153 const auto side3 = vp3.right();
151 154 auto sidepos3 = side3.toGlobal( ROOT::Math::XYZPoint{0, 0, 0} );
152 155 print_position( slice3_id, "Right Side", sidepos3 );
153 auto diff3 = sidepos3 - ROOT::Math::XYZPoint{0.6, 0.6, 0.6};
154 if ( diff3.Mag2() > 0.001 ) { ::exit( EINVAL ); }
156 auto diff3 = sidepos3 - ROOT::Math::XYZPoint{6, 6, 6}; // LHCb units, so in mm
157 if ( diff3.Mag2() > 0.001 ) {
158 std::cout << "position of the right side of the VP wrong : " << sidepos3 << " - expected {6, 6, 6}\n";
  • Rosen Matev
    Rosen Matev @rmatev ·
    Maintainer

    It would be nice to use explicit units:

    Suggested change
    156 auto diff3 = sidepos3 - ROOT::Math::XYZPoint{6, 6, 6}; // LHCb units, so in mm
    157 if ( diff3.Mag2() > 0.001 ) {
    158 std::cout << "position of the right side of the VP wrong : " << sidepos3 << " - expected {6, 6, 6}\n";
    156 auto diff3 = sidepos3 - ROOT::Math::XYZPoint{6 * Gaudi::Units::mm, 6 * Gaudi::Units::mm, 6 * Gaudi::Units::mm};
    157 if ( diff3.Mag2() > 0.001 * Gaudi::Units::mm2 ) {
    158 std::cout << "position of the right side of the VP wrong : " << sidepos3 << " - expected {6 mm, 6 mm, 6 mm}\n";

    but of course the problem is that Gaudi is not a dependency. So can we copy just the SystemOfUnits.h header (not for this MR) and avoid magic constants and "bare" numbers? Of course not for this MR.

  • S
    Sebastien Ponce @sponce ·
    Author Owner

    You got precisely the point. I had left the 0.6 and used Gaudi::Units::cm initially before I was forced to do that when I realized I had no Gaudi. I'm not a fan of duplicating code, so for a test case, I left it like this.

  • C
    Marco Clemencic @clemenci ·
    Owner

    Honestly I would ditch Gaudi::Units in favour of mp-units, but it requires C++20 and it's a massive shift of the way to deal with numbers.

  • Please register or sign in to reply
  • Rosen Matev approved this merge request

    approved this merge request

  • Sebastien Ponce resolved all threads

    resolved all threads

  • Rosen Matev mentioned in issue #10

    mentioned in issue #10

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
    • Please register or sign in to reply