-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
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)
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
TrackTfClassifier
totrackTfClassifierDefault
and then add a new fileRecoTracker/FinalTrackSelectors/plugins/trackTfClassifier_cfi.py
which clonestrackTfClassifier
fromtrackTfClassifierDefault
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- remove RecoTracker/FinalTrackSelectors/plugins/TrackTfClassifier_CKF.cc
- use
trackdnn
as a common modifier andtrackdnn_CKF
to 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() |
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.
please create a new file
) | ||
|
||
trackSelectionTf_CKF = _tfGraphDefProducer.clone( |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as proposed above, use trackdnn_CKF
to swap in qualityCutDictionary_CKF
:
trackdnn_CKF.toReplaceWith(qualityCutDictionary, qualityCutDictionary_CKF)
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.
There is error that toReplaceWith does not work with type <class 'dict'>
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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