Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CTPPS Sim and Reco: Change of paradigm: oldDD and DD4hep in parallel #31383

Merged
merged 43 commits into from
Sep 15, 2020

Conversation

ghugo83
Copy link
Contributor

@ghugo83 ghugo83 commented Sep 7, 2020

PR description:

This is a change of paradigm of #31240 , which was fully porting PPS to rely on DD4hep.

Here instead, old DD scenarios and geometry building are restored and used by default in CTPPS Sim and Reco.
old DD scenarios are not affected in any way.

DD4hep scenarios and geometry building are added, and can be used in CTPPS simulation and reconstruction steps instead, in test files, by loading the dd4hep scenarios instead (in Geometry.VeryForwardGeometry.dd4hep).

The legacy geometry building code is reshaped, so that the code for building geometry with old DD and DD4hep is in common. A fromDD4hep boolean is used when necessary.

NB: This includes few fixes (in legacy code) mentioned in #31297 , at the occasion of previous PR.
NB 2 : The commits are from the last 3 days, and allow full change of paradigm.
NB 3: If any issue, time to say it now :)
NB 4: Please keep comments related to this PR, I cannot rewrite full PPS packages legacy code unfortunately ;)

@jan-kaspar @wpcarvalho @mundim @forthommel @malbouis @fabferro @dpiparo @cvuosalo @ianna @civanch

More details, on dedicated topics, in sections below.

[1] Scenarios available for both oldDD and DD4hep:

The scenarios for old DD are still in Geometry/VeryForwardGeometry, and are untouched.
The scenarios for DD4hep are in Geometry.VeryForwardGeometry/dd4hep.

XMLs-based scenarios:

  • geometryPPS_CMSxz_fromDD_2016_cfi
  • geometryPPS_CMSxz_fromDD_2017_cfi
  • geometryPPS_CMSxz_fromDD_2018_cfi
  • geometryRPFromDD_2017_cfi
  • geometryRPFromDD_2018_cfi

DB scenario:

  • geometryRPFromDB_cfi

[2] PR validation:

Checked no regression in old DD + Checked no difference between oldDD and DD4hep.

  • All workflows are untouched and rely on old DD.

  • No diff on any geo info, when using DD4hep. (volume names, materials, placements, shapes, sensor types, DetIds, etc).
    Fully tested on all volumes and all 6 scenarios.
    Max diff in placement is 1 um.
    DD4hep is only used by test files anyway.

  • No regression in reconstruction, when using DD4hep. (tests scripts from @jan-kaspar , big thanks!):
    "Direct" simulation, using geometry from XML files: make_cmp_simu.pdf
    Reconstruction of LHC data (RAW to proton), using geometry from DB: make_cmp_reco.pdf
    DD4hep is only used by test files anyway.

[3] PPS geometry building re-organization:

PPS geometry building legacy code was heavily duplicated.
Hence, a first step was to reorganize CTPPS geometry building, to remove duplicated functionalities.
Now, generic geometry building code is used both by old DD and DD4hep.

[4] DD4hep migration:

  • Switch to DD4hep-based versions of DDCompactView and DDFilteredView.
  • DDTranslation → dd4hep::Direction (ROOT::Math::XYZVector).
  • DDRotationMatrix → dd4hep::Rotation3D (ROOT::Math::Rotation3D).
  • Fixed name casting regressions (due to namespace not appearing with DD4hep).
  • Fixed sensor type computation regressions (due to namespace not appearing with DD4hep). Use a SpecPar section to select the 2x2:RPixWafer volumes.
  • Fixed ID computation regressions (due to different order of parent volumes copy numbers with DD4hep).
  • Fixed shape parameters regressions (due to different order / units with DD4hep). Only uses the Box parameters. Struct to access data.
  • Debug and fixed units. Since mm was the reference until now, by fear of regression, CTPPS team (with @wpcarvalho) has decided to keep mm as reference. Hence conversions as needed.

davidlange6 and others added 28 commits September 3, 2020 07:54

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…data/CTPPS_Pixel_Module_2x2.xml. Moved into a dedicated file, only included in DD4hep scenarios.
…cD, nD are hard to understand for reader). deque of observing raw pointers (ideal geo) are kept. For the aligned geo, one would need to change evth in DetGeomDesc children to be able to use smart pointers directly. For aligned geo, the children are deleted anyway, when alignedDetRoot is destructed (a DetGeomDesc object destructor calls destructors of its children DetGeomDesc).
…consistent to have both old DD and DD4hep in parallel.
…tGeomDesc.h without having a dependency mess, so just do the construction in the caller code.
… without any code duplication. The old DD code for DetId computation is identical (apart from cleaning style).
…ons as a private function to avoid further issues + provide DetGeomDesc comparator.
…S_final"

This reverts commit 2c7b8d6, reversing
changes made to 3173c7f.
@cmsbuild cmsbuild added this to the CMSSW_11_2_X milestone Sep 7, 2020
@jfernan2
Copy link
Contributor

Thanks @kpedro88 yes, the number agree

@jfernan2
Copy link
Contributor

+1

@cvuosalo
Copy link
Contributor

+1

@jpata
Copy link
Contributor

jpata commented Sep 14, 2020

+reconstruction

  • changes to reco packages are minor and understood
  • reworks the previous PR Port CTPPS to DD4hep #31240, keeping oldDD and DD4HEP in parallel

(same as #31383 (comment))

@civanch
Copy link
Contributor

civanch commented Sep 14, 2020

+1

@tlampen
Copy link
Contributor

tlampen commented Sep 15, 2020

+alca

@civanch
Copy link
Contributor

civanch commented Sep 15, 2020

@ggovi , can you, please, sign this PR?

@ggovi
Copy link
Contributor

ggovi commented Sep 15, 2020

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet