-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Phase2 Tracker description change of paradigm, to be compatible with DD4hep #32843
Conversation
…library paradigm.
… detector itself is identical to T21. The description should be compatible with both DDD and DD4hep.
…on is D77, description is T24. Description is the first compatible with both old DD and DDDhep. Running a Phase 2 DD4hep workflow on any other geometry scenarion does not make any sense, as the descriptions were incompatible with DD4hep.
… Outer Trackers, as evth is identical from OT cabling map point of view.
…mes are distinct for flipped and unflipped ladders. Here, the active volume names are the same anyway (hence all volumes are properly selected).
…ht can check for regression), but Tracker envelope changed to leave space for FCBM.
code-checks |
@ghugo83 For the new XML files this PR is adding, shouldn't they have a "v1" in their path names? Might there be revisions with the need to preserve the previous version? For example, the first new XML file could be called:
I'm not sure that it is desirable to include the date in the directory name. What if one file in the directory changes while the other does not? Does a new directory need to be created? |
@cmsbuild, ping? |
@smuzaffar it seems that running the code checks is taking forever here. Maybe there's something stuck? |
In Phase II, like for Run3, we never modify existing XMLs, but add the XMLs which are different in a repo (only the files which differ), and add a dedicated Tracker version (T21, T22, T23, T24, etc). For example here, the next PR will be integrating the latest coordinates from TFPX CAD, which would not make sense to have as a v2, but more as a Tracker_TFPX dedicated repo. Anyway, both systems are ok in my opinion, would not really bother with this type of details now. We would need to focus on making sure there is no regression with DD4hep (for DDD should be ok). |
code-checks |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32843/21067
|
Should be done with next pre-release. |
This does not modify the geometry itself. It provides a description of Phase 2 Tracker (same detector design as T21), which is compatible with both DDD and DD4hep formats. The new version has been heavily validated locally. |
+Upgrade @ghugo83 Thanks. We should converge on a plan with @cms-sw/pdmv-l2 , i.e. what to be done with pre4. If it is identical, I don't see the point to do D76 for the next pre-release. We may go straight to D77, with your extra validation between D77-D76. This is up to you. |
Ok sounds like a good plan. Whatever you prefer, whatever is easier. |
What? Could you come with a clear plan before? We just submitted D76 in 11_3_0_pre3. Are we now moving to D77? We have many things in hand and not able to have D76-D49, D77-D76 validations as you like...excuse me? |
@chayanit @ghugo83
|
at the moment, the validation of the geometry resulting from PR is not a top priority for the Tracker group. |
OK, I think we should stay with the original plan of D76. Do I understand correctly that this PR is only DD4HEP vs DDD for Tracker? If so, I think we should switch to D77 only after we move to DD4HEP for Phase-2 and perform global validation for DD4HEP-DDD. As far as I understood, we are focusing DD4HEP for Run3 at the moment (?). I'm still confused with the plan of DD4HEP though. FYI. @silviodonato , @qliphy |
+operations |
I agree that DD4hep should only become the default for Phase2 after completing extensive validation. We cannot block ongoing development in many other areas. |
Yes, this is not about shifting Phase 2 workflows to DD4hep, but just doing changes in the XMLs, so that the XMLs descriptions are ready, whenever that happens in the future. Hence, this does not block anything. |
@cms-sw/pdmv-l2 @cms-sw/l1-l2 do you have further comments? |
+1 |
merge |
The previous Phase 2 Tracker descriptions were done for DDD only, and are incompatible with DD4hep library.
This provides an entire review of the Phase2 Tracker description, to be compatible with dd4hep.
The changes introduced in this PR are listed in [1].
To be noted is that the new description XML set is compatible with BOTH DDD and DD4hep.
The new Tracker description is based on T21.
Geometry scenario: 2026D77.
Associated workflows: 350xx (no PU), 352xx (PU).
There is no change in the Tracker design itself incorporated here, to be able to properly check for any regression. A comparison should be made in DDD mode, between D76 (T21) and D77 (T24).
There should be no diff in DDD mode, between D76 and D77.
Instead, in DD4hep mode, the Tracker runs are entirely fixed: the new XML set fixes issues in geometry & materials, associated with incompatibilities with DD4hep library.
[1] Changes incorporated in this PR:
A hierarchy level was introduced in the Composite Materials creation core code in tkLayout, then propagated back to the XML export, and then finally to the XML description.
This allows to have the callees composites always defined above the callers composites in the XMLs description.
Otherwise, the DD4hep G4Materials converter would create WRONG compositions.
There were no unit specified for radii and Z in algorithms arguments, while these lengths should be mm. This was wrongly interpreted, in the DD4hep case, as cm.
In the Phase 2 Tracker, the namespace is used to distinguish OT and IT volumes, in SpecPars.
For example,
tracker:Layer1
versuspixel:Layer1
.With DD4hep, when processing the topology, the namespace are removed. Hence, there was no way to distinguish volumes properly, hence the navigation could not work for Phase 2 (already fixed for Phase 1).
This PR introduces all necessary information directly inside the volume names.
The namespaces are then removed from the SpecPar paths.
The fact that the names are reworked, allows to also significantly shorten paths, for example in
trackerRecoMaterial.xml
.Also profited from this PR, to include a change in Tracker envelope, to leave space for FBCM detector. This was included, since it does not imply any change to the detector description itself.
Selection of diffs introduced in this PR:
PR validation:
Following was done for validation:
TO DO: compare DDD D66 versus DDD D67 further downstream.
Do not expect any specific issue (took care of not changing strings like
BModule
for example, since it is used in Geometry/TrackerGeometryBuilder).But this cannot be done fully blindly, need to be checked.
NBs
NB 1: DD4hep D67 worflow does not run smoothly, due to non-related issues in HGCAL. I was able to debug the DD4hep D67 description by plugging it to a Run 3 Tracker envelope.
NB 2: No specific focus on performance here, the idea in this PR was to get the run working on a DD4hep Phase 2 Tracker first, by fixing the numerous issues. Should expect additional perf improvement to come (spotted several points inside GeometricDet).
NB 3: The changes are incorporated within tkLayout materials description core code, and tkLayout XML export, ie any next Phase 2 Tracker description, will now automatically rely on this new paradigm.
FYI: @ianna @emiglior @skinnari @mmusich @jalimena @fabiocos @cvuosalo @civanch