[DDD] Fix building of DDD GEM reco geometry DB payload #36869
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36869/28128
|
|
A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master. It involves the following packages:
@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
type bug-fix |
|
@cmsbuild please test |
format fix warning Process demonstrator geometry also for database geometry Remove workaround for demonstrator geometry in conddb builder Update Geometry/GEMGeometryBuilder/src/GEMGeometryParsFromDD.cc Stop iterating once we know we are not in the demonstrator geometry Co-authored-by: Carl Vuosalo <cvuosalo@users.noreply.github.com> Add Carl's suggestions Revert DD building changes, should be covered by cms-sw#36869
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a8804a/22175/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
+1 |
|
@cvuosalo do you understand the supposed (?) differences in the MTD vertices simPVZ in the DQM comparisons? |
|
@perrotta This files in this PR are not used by any workflow, so this PR cannot have any effect on the DQM comparisons. Any differences are unrelated to this PR. |
The ones visible in the automatic test outputs: I also tend to believe it is a false positive, histograms look empty anyhow |
| doSuper = fv.firstChild(); | ||
| if (doSuper) { | ||
| GEMDetId detIdCh = | ||
| GEMDetId(gemNumbering.baseNumberToUnitNumber(muonDDDNumbering.geoHistoryToBaseNumber(fv.geoHistory()))); |
There was a problem hiding this comment.
It's a minor comment on the code style. On new GE2/1, we define rawidCh first, then use it once. In this part, we don't define new parameter, we directly get detIdCh.
There was a problem hiding this comment.
I think I prefer the two-step process of getting the rawidCh first and then constructing the GEMDetId, but either style is OK.
This piece of code is for building the GEM reco geometry DB payload with DDD. I hope that we never have to do this again. Eventually, all DDD-related code is slated for deletion.
|
+Upgrade This PR is to fix the building of DDD GEM RECO geometry to DB. It will not affect physics, so no change is expected. PR test runs fine. My comment is considered to be minor in the code style. |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
@srimanob This PR changes the tool I run manually to create the GEM reco geometry DB payload. The code in this PR is never used anywhere else. It has nothing to do with Phase 2 or MTD. The PR tests are actually irrelevant to this PR because it doesn't affect any workflows. Any differences in the PR tests come from some fluctuation in the test themselves or some issue in the MTD code unrelated to this PR. |
|
+1 |
[DDD] Fix building of DDD GEM reco geometry DB payload, backport of #36869
PR #36835 fixed the building of the GEM reco geometry for DD4hep, but it caused a problem for building the DDD GEM reco geometry DB payload. Technically, an unreliable
DDFilteredViewcopy constructor had been used, which caused the originalDDFilteredViewto be changed when the copy was altered. The solution is to create a separate, identicalDDFilteredViewthat can be iterated through without altering the original.Note that this PR only affects the manual process of creating the DDD GEM reco geometry DB payload, which is done by an expert. This code is never used in any workflow.
PR validation:
A correct DDD GEM reco geometry DB payload was created successfully with this PR, whereas previously the creation program crashed.
if this PR is a backport please specify the original PR and why you need to backport that PR:
For completeness and consistency, this PR could be backported to 12_2, but it is not strictly necessary. There shouldn't be a need to create geometry DB payloads in 12_2, since the latest 12_3 tags are the ones that should be used with 12_2_1.