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

Phase2 Tracker description change of paradigm, to be compatible with DD4hep #32843

Merged
merged 11 commits into from Feb 18, 2021

Conversation

ghugo83
Copy link
Contributor

@ghugo83 ghugo83 commented Feb 8, 2021

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:

  • Ordering of composites materials:
    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.
  • Units
    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.
  • Namespace removal in all SpecPar paths, and corresponding change of all volume names
    In the Phase 2 Tracker, the namespace is used to distinguish OT and IT volumes, in SpecPars.
    For example, tracker:Layer1 versus pixel: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.
  • Simplify paths:
    The fact that the names are reworked, allows to also significantly shorten paths, for example in trackerRecoMaterial.xml.
  • Tracker envelope:
    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:

  • Checked XMLs make sense from Tracker design point of view.
  • 0 overlap within full 'Tracker' volume (checked with Fireworks + Geant4 tools).
  • Checked workflow numbering.
  • Checked Material Budget profile: DDD D66 versus DDD D67, and DDD D67 versus DD4hep D67.
  • Checked all GeometricDet member data: DDD D66 versus DDD D67, and DDD D67 versus DD4hep D67.
  • Checked that workflows with DDD D67 run smoothly with no extra error / warning (versus DDD D66).

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

… 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.
@mmusich
Copy link
Contributor

mmusich commented Feb 8, 2021

code-checks

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 8, 2021

@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:

 Geometry/TrackerCommonData/data/PhaseII/Tracker_DD4hep_compatible/v1/pixel.xml

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?

@mmusich
Copy link
Contributor

mmusich commented Feb 9, 2021

@cmsbuild, ping?

@mmusich
Copy link
Contributor

mmusich commented Feb 9, 2021

@smuzaffar it seems that running the code checks is taking forever here. Maybe there's something stuck?

@ghugo83
Copy link
Contributor Author

ghugo83 commented Feb 9, 2021

@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:
Geometry/TrackerCommonData/data/PhaseII/Tracker_DD4hep_compatible/v1/pixel.xml
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?

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).
Unlike Run 3, where usually very small changes are integrated, the addition usually represent a big feature. Hence the version variants v1, v2, v3 etc would not be very meaningful here.

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).

@smuzaffar
Copy link
Contributor

code-checks

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2021

@srimanob
Copy link
Contributor

srimanob commented Feb 13, 2021

@ghugo83 @mmusich

Is the following statement, in the PR description,
"A comparison should be made in DDD mode, between D76 (T21) and D77 (T24)."
done privately, or to be done when the next pre-release will be available?

If done, could you please point to the results?

Thanks very much.

@ghugo83
Copy link
Contributor Author

ghugo83 commented Feb 15, 2021

@ghugo83 @mmusich
Is the following statement, in the PR description,
"A comparison should be made in DDD mode, between D76 (T21) and D77 (T24)."
done privately, or to be done when the next pre-release will be available?
If done, could you please point to the results?
Thanks very much.

Should be done with next pre-release.

@civanch
Copy link
Contributor

civanch commented Feb 15, 2021

@srimanob, @ghugo83 , we just have pre-release pre3. This PR provides modified geometry, which can be compared using DDD. Is this correct?

@ghugo83
Copy link
Contributor Author

ghugo83 commented Feb 15, 2021

@srimanob, @ghugo83 , we just have pre-release pre3. This PR provides modified geometry, which can be compared using DDD. Is this correct?

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.
For further validation (check no impact on tracking performance), one could compare samples produced with T21 and this description (T24) (since they correspond to the same detector design, but with different descriptions formats). Though, that is not fully necessary, as any issue would most likely have already appeared at step 1, which was heavily validated. This would just be to go from the 99.9 certainty to the 100.

@srimanob
Copy link
Contributor

+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.

@ghugo83
Copy link
Contributor Author

ghugo83 commented Feb 15, 2021

We may go straight to D77, with your extra validation between D77-D76.

Ok sounds like a good plan. Whatever you prefer, whatever is easier.

@chayanit
Copy link

+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.

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?

@srimanob
Copy link
Contributor

@chayanit @ghugo83
I don't know until the PR arrives. I think the question to discuss is that do we really need D77 in form of DDD at this point?

  • Can we keep using D76 until we move Phase-2 to DD4hep?
  • Could tracker confirm the transition to DD4hep is done, i.e. Is validation of D76 (DDD) - D77 (DD4hep) enough? ==> Can we do limited validation sample for tracker, i.e. few NoPU samples for tracker only?
  • Can we validate once in that time we do DD4hep - DDD validation?

@mmusich
Copy link
Contributor

mmusich commented Feb 15, 2021

@srimanob

at the moment, the validation of the geometry resulting from PR is not a top priority for the Tracker group.
I'd rather stay with D76 until the that's done, which might be pre4 or later depending on the availability of PdmV

@chayanit
Copy link

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

@silviodonato
Copy link
Contributor

+operations

@kpedro88
Copy link
Contributor

I agree that DD4hep should only become the default for Phase2 after completing extensive validation. We cannot block ongoing development in many other areas.

@silviodonato
Copy link
Contributor

@chayanit right we are focusing on Run-3 about DD4HEP. As @ghugo83 wrote in the description, this is a step forwards the DD4HEP migration of the Phase-2 tracker and it supports both DDD and DD4HEP.
I think it is good to decouple this development from D76, so I'm happy to have a new D77

@ghugo83
Copy link
Contributor Author

ghugo83 commented Feb 15, 2021

I agree that DD4hep should only become the default for Phase2 after completing extensive validation.

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.

@silviodonato
Copy link
Contributor

@cms-sw/pdmv-l2 @cms-sw/l1-l2 do you have further comments?

@chayanit
Copy link

+1

@silviodonato
Copy link
Contributor

merge
cc @rekovic

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