Phase 1 Pixel Validation code for CMSSW_9_2_X to replace Phase 0 Pixel Validation code for Phase 1 Pixel era. #19403
Conversation
|
A new Pull Request was created by @davidcarbonis (Alexander Morton) for master. It involves the following packages: Validation/Configuration The following packages do not have a category, yet: Validation/SiPixelPhase1ConfigV @perrotta, @civanch, @vazzolini, @kmaeshima, @mdhildreth, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
|
| edm::Handle<edm::SimTrackContainer> simTrackCollection; | ||
| iEvent.getByToken(simTracksToken_, simTrackCollection); | ||
| if ( simTrackCollection.isValid() ) { | ||
| simTC = simTrackCollection.product(); |
There was a problem hiding this comment.
This survives and leaks outside analyze(). Please remove. The variable looks to be unused.
There was a problem hiding this comment.
Same applies to associatorByHits, it should not be a class member!
There was a problem hiding this comment.
Sorry for the delay in getting around to fixing these.
You are correct re. simTrackCollection not being used any longer. It appears I forgot to clean up unused bits of code from previous implementation attempts.
You are correct that associatorByHits is not required as a class member. It has been removed. Lines 228-232 are kept though as the variable is used for the reco to sim hit association.
These changes will be committed in the next few minutes.
cmsbuild
added
comparison-pending
reconstruction-pending
and removed
comparison-available
reconstruction-approved
tests-approved
labels
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
+1 |
|
This appears to be causing many segmentations faults in the RelVal workflows. E.g. |
|
@Dr15Jones: Found where things break. Looking into why - will let you know asap and will open a PR with the fix required. |
|
PR: #19811 solves the seg faults observed. My apologies @Dr15Jones for missing such an obvious check. |
| } | ||
|
|
||
| edmNew::DetSetVector<SiPixelCluster>::const_iterator it; | ||
| for (it = clusterColl->begin(); it != clusterColl->end(); ++it) { |
There was a problem hiding this comment.
is this loop supposed to be inside the loop over tracks?
It seems like it should be outside.
[I arrived here after noticing that this module takes twice as much as all of reco in PU35]
There was a problem hiding this comment.
Hi think this loop is here because originally the code was able to separate on-track and off-track clusters, however we decided to drop the off track clusters plots at a certain point and this loop should no more be useful. @davidcarbonis please check.
There was a problem hiding this comment.
@fioriNTU you are correct regarding the code formerly separating on- and off-track clusters and the consequently the loop no longer being useful. I'll open a PR to resolving this later today.
There was a problem hiding this comment.
@slava77 would a backport to CMSSW_9_3_X be more appropriate than a merge into the master base?
There was a problem hiding this comment.
Hi @davidcarbonis , a PR to master is mandatory , so please make a PR there. Then you should backport - Thanks
There was a problem hiding this comment.
Cheers for clarifying @boudoul :)
There was a problem hiding this comment.
I don't expect a 93X backport to be really necessary at this point.
93X is used for 2017 GEN-SIM and for phase-2 endcap TDR. Neither have this module running.
There was a problem hiding this comment.
Hi @slava77 , thnaks, I was thinking of a backport just to save some time in std workflows ran in IB tests, PR tests etc..
Revision of closed PRs: #18435 and #19175 . Small refinements added (such as suppressing phase 0 pixel validation output in phase 1 pixel era) and the recent DQM client changes (as the code inherits from the phase 1 pixel DQM framework). Binning for the new Hits histograms has been optimised to be of the same order of magnitude as those plots it replaces.
Added validation modules for the phase 1 pixel code based off the phase 1 dqm framework. Replicates the functionality of the phase 0 code, namely:
Digis
Hits
RecHits
Track Clusters
Tracking MC Truth (referred to as TrackingParticle in the phase 1 validation directory structure)
Validation/Configuration/python/globalValidation_cff.py; Validation/Configuration/python/postValidation_cff.py;
and Validation/Configuration/python/trackerSimValid_cff.py; have been modified so that if the era is phase1Pixel, the new validation code is used.
When in the phase 1 pixel era, the phase 0 pixel validation output is disabled. The phase0 validation has been kept in for cases where it may be needed in the future (such as a new rereco).
@fioriNTU @lunik1 @ckmackay - you might be interested in this.
@davidlange6 - Number of removed histogram bins from Phase 0 Validation for Pixel Tracker:
TrackerDigis: ~19,080
TrackerHits: ~1,440,000
TrackerRecHits: ~16,170
Total removed: ~1,475,250
Number of added histogram bins for Phase 1 Validation for Pixel Tracker:
SiPixelPhase1DigisV: ~841,600
SiPixelPhase1HitsV: ~5,006,800
SiPixelPhase1RecHitsV: ~981,640
SiPixelPhase1TrackClustersV: ~273,520
SiPixelPhase1TrackingParticleV: ~5,130
Total added: ~7,108,690
Total difference: +5,633,440 added
N.B. Some plots in TrackerHits (~4,000,000 bins) and all plots in TrackingMCTruth (~1,630 bins) contain both pixel and strip information, and thus those bins are still filled (and not counted as removed above). In the case of the TrackerHits plots, only strip information is filled, but both pixel and strip information is still filled for TrackingMCTruth.