Track DNN update on mkFit tracks #35686
Conversation
|
@minxiyang, CMSSW_12_1_X branch is closed for direct updates. cms-bot is going to move this PR to master branch. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35686/25979
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
|
A new Pull Request was created by @minxiyang (Minxi Yang) for master. It involves the following packages:
@perrotta, @cmsbuild, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
For reference, this PR needs the DNN files from cms-data/RecoTracker-FinalTrackSelectors#10 |
|
Please update the PR title (e.g. "Track DNN update on mkFit tracks" or similar, to be clearer) and the PR description (adding links to relevant indico presentations and the required data PR). |
|
test parameters:
|
| #include "RecoTracker/FinalTrackSelectors/interface/TfGraphDefWrapper.h" | ||
|
|
||
| namespace { | ||
| class TfDnn_CKF { |
There was a problem hiding this comment.
why is this file needed? all parameters and implementation appear to be the same as TrackTfClassifier. It seems like a simple instance of TrackTfClassifier would work.
There was a problem hiding this comment.
- remove RecoTracker/FinalTrackSelectors/plugins/TrackTfClassifier_CKF.cc
actually, if we go this way, the more appropriate additional steps would be to change the name of the default cfi file in the fillDescriptions of
TrackTfClassifiertotrackTfClassifierDefaultand then add a new fileRecoTracker/FinalTrackSelectors/plugins/trackTfClassifier_cfi.pywhich clonestrackTfClassifierfromtrackTfClassifierDefaultand usestrackdnn_CKF.toModify(trackTfClassifier, tfDnnLabel = "trackSelectionTf_CKF")
I am not fully understand this comment. Does it add a new file RecoTracker/FinalTrackSelectors/python/trackTfClassifier_cfi.py? When I should call this file? Instead of it, can I directly add trackdnn_CKF.toModify(trackTfClassifier, tfDnnLabel = "trackSelectionTf_CKF") in all the iteration_cff files for example RecoTracker/IterativeTracking/python/DetachedQuadStep_cff.py?
There was a problem hiding this comment.
Does it add a new file RecoTracker/FinalTrackSelectors/python/trackTfClassifier_cfi.py? When I should call this file?
Yes, a new file trackTfClassifier_cfi.py is needed, it will contain
import RecoTracker.FinalTrackSelectors.trackTfClassifierDefault_cfi as _mod
trackTfClassifier = _mod.trackTfClassifierDefault.clone()
trackdnn_CKF.toModify(trackTfClassifier, tfDnnLabel = "trackSelectionTf_CKF")Instead of it, can I directly add
this is more complicated, IMHO.
There was a problem hiding this comment.
trackdnn_CKF.toModify(trackTfClassifier, tfDnnLabel = "trackSelectionTf_CKF")
It doesn't work. 'Unknown parameter name tfDnnLabel specified while calling Modifier'
I think it is because there is the namespace MVA for the desc for tfDnnLabel. However, I try the mva.tfDnnLabel, and MVA.tfDnnLabel. And both doesn't work
There was a problem hiding this comment.
trackdnn_CKF.toModify(trackTfClassifier.mva, tfDnnLabel = "trackSelectionTf_CKF")
?
sorry, I didn't check the exact placement of the parameter in the original comment
There was a problem hiding this comment.
- remove RecoTracker/FinalTrackSelectors/plugins/TrackTfClassifier_CKF.cc
- use
trackdnnas a common modifier andtrackdnn_CKFto revert to fully CKF setup - the parts of Enable mkFit by default in 4+2 tracking iterations (initialStepPreSpl… #35660 should probably be removed to avoid conflicts
I did not repeat the same comments in the xyzStep_cff files, which I think do not need any modifications in this PR.
| @@ -2,3 +2,4 @@ | |||
|
|
|||
| # This modifier sets the used tracking classifier to be a deep neural network instead of the BDT | |||
| trackdnn = cms.Modifier() | |||
| trackdnn_CKF = cms.Modifier() | |||
| ) | ||
|
|
||
| trackSelectionTf_CKF = _tfGraphDefProducer.clone( |
There was a problem hiding this comment.
cfi files should normally have just one module defined.
Please create a trackSelectionTf_CKF_cfi.py
| trackdnn.toReplaceWith(detachedQuadStep, TrackTfClassifier.clone( | ||
| src = 'detachedQuadStepTracks', | ||
| qualityCuts = qualityCutDictionary['DetachedQuadStep'] | ||
| )) | ||
| trackdnn_CKF.toReplaceWith(detachedQuadStep, TrackTfClassifier_CKF.clone( | ||
| src = 'detachedQuadStepTracks', | ||
| qualityCuts = qualityCutDictionary_CKF['DetachedQuadStep'] | ||
| )) |
There was a problem hiding this comment.
I see that here and in other files the pair is repeated.
I think that it's more practical to modify qualityCutDictionary specifically with trackdnn_CKF where the qualityCutDictionary is defined.
Due to very significant commonality, I have a mild preference to treat trackdnn as a common modifier and use trackdnn_mkFit and trackdnn_CKF to add specific variations on top.
Actually, IIUC, mkfit-specific and trackdnn_CKF parts are currently exclusive; so then there is no need to add trackdnn_mkFit. And trackdnn_CKF can be treated as add-on/toggle to swap CKF-specific parts where they have to replace the mkFit parts (and if need be ~trackdnn_CKF can be used).
This way this file and the other xyzStep_cff.py files do not need to be modified by this PR.
| src = 'detachedTripletStepTracks', | ||
| qualityCuts = qualityCutDictionary_CKF['DetachedTripletStep'], | ||
| )) | ||
| (trackdnn_CKF & trackdnn & fastSim).toModify(detachedTripletStep,vertices = 'firstStepPrimaryVerticesBeforeMixing') |
There was a problem hiding this comment.
shouldn't this be an .OR. ?
| (trackdnn_CKF & trackdnn & fastSim).toModify(detachedTripletStep,vertices = 'firstStepPrimaryVerticesBeforeMixing') | |
| ((trackdnn_CKF | trackdnn) & fastSim).toModify(detachedTripletStep,vertices = 'firstStepPrimaryVerticesBeforeMixing') |
Note, however the comment in RecoTracker/IterativeTracking/python/DetachedQuadStep_cff.py
If the trackdnn can become just a common modifier, then this file will not need modifications.
BTW, how did it happen that this file changes the vertices for (trackdnn & fastSim) while the detachedQuad does it generically for fastSim ?
| } | ||
|
|
There was a problem hiding this comment.
as proposed above, use trackdnn_CKF to swap in qualityCutDictionary_CKF:
trackdnn_CKF.toReplaceWith(qualityCutDictionary, qualityCutDictionary_CKF)There was a problem hiding this comment.
There is error that toReplaceWith does not work with type <class 'dict'>
There was a problem hiding this comment.
ah, right, the values will need to be wrapped in a PSet to be able to apply the modifiers
smth like
qualityCutDictionary = cms.PSet(
InitialStep = cms.vdouble(-0.48, 0.03, 0.25),
...
)
trackdnn_CKF.toModify(qualityCutDictionary,
InitialStep = [-0.49, 0.08, 0.34],
...and then in the xyzStep files it will need an update to qualityCuts = qualityCutDictionary.InitialStep
There was a problem hiding this comment.
qualityCuts = qualityCutDictionary.InitialStep
qualityCuts = qualityCutDictionary.InitialStep.value() would be better so initialStep.qualityCuts and and qualityCutDictionary.InitialStep would stay as distinct objects.
| trackdnn_CKF.toReplaceWith(iterTrackingTask, _iterTrackingTask_trackdnn | ||
| ) |
There was a problem hiding this comment.
edits in this file are also not needed if trackdnn is treated as a common mod
| from Configuration.Eras.Era_Run3_cff import Run3 | ||
| from Configuration.Eras.ModifierChain_trackingMkFitProd_cff import trackingMkFitProd | ||
|
|
||
| Run3_noMkFit = Run3.copyAndExclude([trackingMkFitProd]) | ||
| Run3_noMkFit = Run3.copyAndExclude([trackingMkFitProd, trackdnn]) |
There was a problem hiding this comment.
| Run3_noMkFit = Run3.copyAndExclude([trackingMkFitProd, trackdnn]) | |
| Run3_noMkFit = cms.ModifierChain(Run3.copyAndExclude([trackingMkFitProd]), trackdnn_CKF) |
for the proposal of trackdnn to be a common modifier
There was a problem hiding this comment.
I think this change would use trackdnn_CKF as the default for the noMkFit case, but do we plan to do this?
There was a problem hiding this comment.
I think this change would use trackdnn_CKF as the default for the noMkFit case, but do we plan to do this?
The Era is here and has just noMkFit in the name, which means that I'd expect that everything else is the same as in Run3, which includes a flavor of trackdnn.
Perhaps your comment is a reason to introduce Run3_no_trackdnn.
| trackingMkFitDetachedTripletStep, | ||
| trackingMkFitPixelLessStep, |
There was a problem hiding this comment.
can edits in this file be removed to avoid conflicts with #35660 ?
actually, if we go this way, the more appropriate additional steps would be to change the name of the default cfi file in the fillDescriptions of |
| 'DetachedTripletStep': [-0.52, 0.04, 0.76], | ||
| 'PixelPairStep': [-0.47, -0.33, -0.05], | ||
| 'MixedTripletStep': [-0.87, -0.61 ,-0.13], | ||
| 'PixelLessStep': [-0.77, -0.67, -0.48], |
There was a problem hiding this comment.
@minxiyang, could you update the DNN cuts for PixelLessStep to [-0.20, 0.10, 0.40]?
This would allow a better performance, as shown here (black vs. red):
http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_devs/PRvalidation_Oct13/MTV_mkFit-6iter_PR35686_DNNcut/plots_highPurity/effandfakePtEtaPhi.pdf
There was a problem hiding this comment.
this update was chosen to have similar FR points as they were in CKF (0.6, 0.5, 0.35), which correspond to a cut on DNN at (-0.2, 0.1, 0.4) (instead of -0.77, -0.67, -0.48)
the differences are as expected only in phase-1 workflows. at a very cursory level: in wf 136.874 (EGamma 2018C)
[*] TRK POG talk https://indico.cern.ch/event/1087315/contributions/4570863/attachments/2329716/3969639/trackdnn_PR5.pdf |
|
+reconstruction
@minxiyang please edit the PR description to add link(s) to the POG talk(s) https://indico.cern.ch/event/1087315/contributions/4570863/attachments/2329716/3969639/trackdnn_PR5.pdf and perhaps also to the details of how training was done (was there a twiki?). This would help with self-documentation of the PRs. |
|
enable profiling |
|
@cmsbuild please test to get an incremental difference reference for the technical performance changes |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-05a05a/19819/summary.html CMS StaticAnalyzer warnings: There are 1 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-05a05a/19819/llvm-analysis/esrget-sa.txt for details. Comparison SummarySummary:
|
the results look good:
|
|
Maybe I missed it in this long thread, but besides adding link(s) to the POG talk(s) in the PR description, as asked by Slava, could you also please specify there whether the training was done with mkKFit "4+2" or mkFit "4+3"? Another question: why the parameters listed now in |
the training was done on "4+3"; it is applied to the "4+2" currently in cmssw.
the previous |
It is actually "mkfit 4+3" but it was shown at the POG meeting that the penalty performance should not be worrisome.
IIUC the training was changed w.r.t the original implementation by Joona back in 2018 @JanFSchulte |
Indeed, the training and selection Joona integrated before he left turned out to be not optimal and we had to re-do it with proper Run 3 training samples. |
|
+operations |
|
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) |
|
+1 |
PR description:
Set DNN trained with MkFit tracks instead of the BDTs as the default for the MkFit track selection.
Keep BDTs as the default for the CKF track selection.
Add a backup DNN trained with CKF tracks. Include trackdnn_CKF in the process to enable this DNN