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
add validation for GsfTracks @HLT #17447
Conversation
A new Pull Request was created by @mtosi (mia tosi) for CMSSW_9_0_X. It involves the following packages: HLTriggerOffline/Common @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
+1 |
The tests are being triggered in jenkins. |
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
hltGsfTrackValidator.histoProducerAlgoBlock.TpSelectorForEfficiencyVsVTXR.maxRapidity = cms.double( 3.0) | ||
hltGsfTrackValidator.histoProducerAlgoBlock.TpSelectorForEfficiencyVsVTXR.minRapidity = cms.double(-3.0) | ||
hltGsfTrackValidator.histoProducerAlgoBlock.TpSelectorForEfficiencyVsVTXZ.maxRapidity = cms.double( 3.0) | ||
hltGsfTrackValidator.histoProducerAlgoBlock.TpSelectorForEfficiencyVsVTXZ.minRapidity = cms.double(-3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The eta limits for all the TpSelector*
's are already controlled with eras (i.e. set to +-3 for phase1) in
https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/Validation/RecoTrack/python/TrackingParticleSelectionsForEfficiency_cff.py
Please continue using those (if they don't get propagated here for some reason, that should be fixed instead)
The GpSelector*
are not, but the MultiTrackValidatorGenPs
is probably out-of-date anyway (and not used in standard workflows).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @makortel
but here the eta range does not depend on the era, but on the ECAL acceptance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair point. Could you then add a comment explaining that?
Then the above could be also written in a less verbose way
hltGsfTrackValidator = hltMultiTrackValidator.clone(
...,
histoProducerAlgoBlock = dict(
TpSelectorForEfficiencyVsEta = dict(minRapidity=-3, maxRapidity=3),
TpSelectorForEfficiencyVsPhi = dict(minRapidity=-3, maxRapidity=3),
...
)
)
(I'd just ignore the GpSelector*
as they're not used in practice)
hltGsfTrackValidator.histoProducerAlgoBlock.generalTpSelector.maxRapidity = cms.double( 3.0) | ||
hltGsfTrackValidator.histoProducerAlgoBlock.generalTpSelector.minRapidity = cms.double(-3.0) | ||
hltGsfTrackValidator.maxRapidityTP = cms.double( 3.0) | ||
hltGsfTrackValidator.minRapidityTP = cms.double(-3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are also already controlled with eras (set to +-3 for phae1) in
https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/Validation/RecoTrack/python/TrackingParticleSelectionForEfficiency_cfi.py
hltGsfTrackValidator.maxRapidityTP = cms.double( 3.0) | ||
hltGsfTrackValidator.minRapidityTP = cms.double(-3.0) | ||
hltGsfTrackValidator.maxEta = cms.double( 3.0) | ||
hltGsfTrackValidator.minEta = cms.double(-3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two don't do anything, please remove.
(In general, when modifying existing parameter, I'd suggest to avoid the explicit type on right hand side to benefit from typo checking. Without cms.double
it should raise an exception here).
hltTPClusterProducer | ||
+ hltTrackAssociatorByHits | ||
+ hltTrackValidator | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is exactly the same as https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/Validation/RecoTrack/python/HLTmultiTrackValidator_cff.py. Why another copy would be needed?
hltMultiTrackValidator.ignoremissingtrackcollection = cms.untracked.bool(True) | ||
|
||
hltMultiTrackValidator.UseAssociators = True | ||
hltMultiTrackValidator.associators = ['hltTrackAssociatorByHits'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is functionally the same as https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/Validation/RecoTrack/python/HLTmultiTrackValidator_cfi.py. Why another copy would be needed?
label_tp = cms.InputTag("mix","MergedTrackTruth"), | ||
associator = cms.InputTag('hltTrackAssociatorByHits'), | ||
ignoremissingtrackcollection = cms.untracked.bool(True) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used, I'd suggest to remove.
) | ||
from Configuration.Eras.Modifier_phase1Pixel_cff import phase1Pixel | ||
|
||
hltvalidation = cms.Sequence( | ||
HLTMuonVal | ||
+hltMultiTrackValidationGsfTracks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to have all MTV instances in the "prevalidation" Path (i.e. hltassociation
sequence) for consistency (and because there is no reason for these to be in EndPath).
hltTPClusterProducer | ||
+ hltTrackAssociatorByHits | ||
# + cms.ignore(trackingParticlesElectron) | ||
+ hltGsfTracksPreValidation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why hltGsfTracksPreValidation
is inserted in hltMultiTrackValidationGsfTracks
sequence here, while they are inserted in hltassociation
and hltvalidation
sequences in HLTValidation_cff.py
, respectively.
Following my comment there, I'd suggest to remove the hltGsfTracksPreValidation
sequence altogether, uncomment the trackingParticlesElectron
in above, and in HLTValidation_cff.py
insert hltMultiTrackValidationGsfTracks
in hltassociation
.
-1 Tested at: 2536943 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals AddOn
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log135.4 step1 runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log
I found errors in the following addon tests: cmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,HLT:@Fake,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Tue Feb 7 18:38:02 2017-date Tue Feb 7 18:35:04 2017 s - exit: 16640 |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
Pull request #17447 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again. |
adding @rafaellopesdesa |
Pull request #17447 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again. |
Thanks Mia! For FastSim, until #17224 is merged, I guess you would need to remove also |
I think I tested w/ FastSim workflow w/o any issue |
@cmsbuild, please test Ok, let's test it then (earlier there were failures). |
The tests are being triggered in jenkins. |
Comparison job queued. |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
+1 |
as requested by EGM
I'm adding the validation plots for GSF tracks @HLT
@gpasztor @fcouderc @paramatti @dmitrijus