Revision of the PSimHit type storage, revert trackId to its basic meaning#49969
Revision of the PSimHit type storage, revert trackId to its basic meaning#49969fabiocos wants to merge 8 commits intocms-sw:masterfrom
Conversation
…ncies, temporarily silence use of offset to be replaced
…ClassID to set it in TimingSD
Add test for PSimHit storage of process/hit type Enable backward compatibility for PSimHit
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49969/47752 Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49969/47755 |
|
A new Pull Request was created by @fabiocos for master. It involves the following packages:
@Moanwar, @civanch, @cmsbuild, @ctarricone, @gabrielmscampos, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @nothingface0, @rseidita, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
-1 Failed Tests: UnitTests RelVals RelVals-INPUT Failed Unit TestsI found 1 errors in the following unit tests: ---> test testPhase2PixelNtuple had ERRORS Failed RelVals
Expand to see more relval errors ...Failed RelVals-INPUT
|
|
Good point, there does not seem to be any explicit need for |
|
@civanch @kpedro88 to be practical, summarizing:
@makortel based on previous discussion and tests, I see no need to change data member name to have a different checksum, as this seems anyway irrelevant in solving the real pending issue of this PR, i.e. the backward compatibility correct operation. To be clear: backward compatibility is an issue here only for MTD, and only for the performance validation part, where MC truth is used. This is of course very important for our studies, but in principle so far there is no big production, apart for RelVals, where this is present to my knowledge. And code developments should anyway evolve on more recent versions. Still I find quite unpleasant not being able to ensure the proper operation of the schema evolution. But I am running out of ideas about what else I could try. Looking at the crashes, they always mention this statement This PR is never touching the positions that are stored into the object, so I am a bit puzzled. How do you suggest to proceed? This move is not strictly speaking super urgent, but the sooner the better, and for sure before next big MC campaign. |
|
Maybe investigating the crash with ROOT debug build would shed some light? We have a recent 16_1_X ROOT debug build available in cms-sw/cmsdist#10212 (comment) (run the install script to set up CMSSW dev area, cherry-pick your changes and recompile). Ah, this approach would likely "require" recompiling ~everything (all depending code must be, and I'm not sure if it is worth of trying to figure out that set). I guess theoretically we could ask the bot to build this PR with the ROOT debug build option. |
|
@makortel I'll check that |
|
I think the backtrace always shows that line because the first members constructed in PSimHit are: Local3DPoint theEntryPoint; // position at entry
Local3DVector theSegment; // exitPos - entryPosbut I'm not sure why the constructor fails immediately from seemingly innocuous changes. |
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-rejected
code-checks-approved
labels
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49969/47903 Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
@fabiocos , my proposal was different: no friends; make a separate header with static constants, which are included in all classes of interest. |
|
@civanch which contradicts #49969 (comment) and brings us back to the beginning. Please agree among yourself and let me know, otherwise we keep looping. At present there is no new |
fabiocos
force-pushed
the
fc-psimhit-20260122
branch
from
5200ac5 to
74e840e
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49969/47905 |
|
Pull request #49969 was updated. @Moanwar, @civanch, @cmsbuild, @ctarricone, @gabrielmscampos, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @nothingface0, @rseidita, @srimanob can you please check and sign again. |
|
@civanch I strongly prefer the universal mask/shift constants to be members of |
|
please test with cms-sw/cmsdist#10212 |
|
-1 Failed Tests: UnitTests RelVals RelVals-INPUT The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Failed Unit TestsI found 2 errors in the following unit tests: ---> test testPhase2PixelNtuple had ERRORS ---> test testDiMuonBiasesPlotting had ERRORS Failed RelVals
Expand to see more relval errors ...Failed RelVals-INPUT
|
6 participants
PR description:
Following #49732 and the discussion in the Simulation meeting https://indico.cern.ch/event/1634520/contributions/6878108/attachments/3199713/5699141/SIM_20260116.pdf , this PR proposes a revision of the mechanism used to store and propagate the hit type classification, so far used only by MTD for Phase2, but in principle applicable to any interested sub-detector.
trackIdis reverted to the pure Geant4 id, without any offset and limitation on the maximum available (beyond the uint32_t capability), so as not to interfere with exceptionally populated events, and software developments for GPU.The hit type is moved as a 7-bits subfield in the
processTypemember ofPSimHit, where the maximum code is at present 403, and this is a used-defined integer, not a pseudo-random variable. The interface is adapted, the assignment of hit type in MTD is adjusted accordingly, and all the dependencies in the code (truth accumulator, validation) are adapted.PR validation:
Tests on 100 single pions (wf 34506.0) are successful, the detailed Geant4 debug printout shows the desired behaviour, when activated, and the usual DQM histogram test shows perfect comparison between histograms.