-
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
Refactoring of TICL EnergyRegression and PID model #45821
Refactoring of TICL EnergyRegression and PID model #45821
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45821/41561 |
A new Pull Request was created by @Moanwar for master. It involves the following packages:
@Martin-Grunewald, @cmsbuild, @jfernan2, @mandrenguyen, @mmusich, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
test parameters:
|
@cmsbuild please test |
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersPassthrough_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersCLUE3DHighL1Seeded_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTracksterLinks_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersCLUE3DHigh_cfi.py
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersPassthrough_cfi.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,22 @@ | |||
#ifndef RecoHGCal_TICL_TracksterInferenceByANN_H__ |
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.
What is the purpose of this plugin? It is also used in the configurations above, but its implementation is empty.
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 additional empty plugin is set up in anticipation of future models that might arrive later, so we prepared the configuration for it in advance.
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersCLUE3DHighL1Seeded_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersCLUE3DHigh_cfi.py
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersPassthrough_cfi.py
Show resolved
Hide resolved
minNumLayerCluster = cms.int32(5), | ||
type = cms.string('FastJet') | ||
), | ||
pluginInferenceAlgoTracksterInferenceByDNN = cms.PSet( |
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'm confused. I see that here you use the pluginInferenceAlgoTracksterInferenceByDNN
with a TICLv4 model, while in the TracksterProducer
and the linking variant, there is also the plugin TracksterInferenceByCNNv4
. If we configure the DNN
one, should we expect regression?
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.
Hi Marco, so this additional empty plugin is set up in anticipation of future models that might arrive later, so we prepared the configuration for it in advance. Thanks!
HLTrigger/Configuration/python/HLT_75e33/modules/hltTiclTrackstersCLUE3DHigh_cfi.py
Show resolved
Hide resolved
+1 Size: This PR adds an extra 12KB to repository 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:
Comparison SummarySummary:
|
Hi @mmusich, Indeed, this is one of the expected outcomes intended by this new plugin system. First, it allows the dynamic use of different ML models for PID and regression based on the stages, enabling smooth transitions between them as well. Additionally, using ONNX model formats helps optimize prediction time, making this a good result. Thanks! |
+hlt
|
+1 |
+Upgrade |
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. @antoniovilela, @rappoccio, @mandrenguyen, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
Merged cms-data/RecoHGCal-TICL#6 |
+1 |
@Moanwar @cms-sw/hcal-dpg-l2 I suspect this PR might be responsible of widespread failures across builds of the wf
can you have a look? |
@Moanwar it's HFnose reconstruction which is not maintained since a long time. But this does not mean we should break it ;-) You can test it in d115. |
|
||
) | ||
|
||
from Configuration.ProcessModifiers.ticl_v5_cff import ticl_v5 | ||
ticl_v5.toModify(ticlTrackstersCLUE3DHigh.pluginPatternRecognitionByCLUE3D, computeLocalTime = cms.bool(True)) | ||
ticl_v5.toModify(ticlTrackstersCLUE3DHigh.pluginPatternRecognitionByCLUE3D, usePCACleaning = cms.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.
Why was usePCACleaning
set to False?
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.
@sameasy fyi
This PR adds CNN models trained using TICLv5 reconstruction information through a simple CNN-based approach. Two separate models were developed: one for trackster energy regression and another for particle ID. These models are integrated into different reconstruction steps, specifically CLU3D (pattern recognition) and trackster linking. The models are saved in ONNX format to optimize processing time.
Additionally, we have moved the inference to the TracksterProducer and TracksterLinksProducer, instead of running the inference within the algorithm. This was achieved by creating a new TICL inference plugin system, which allows for the inclusion of more models without conflicting with the old ones. We have also converted the old TensorFlow model of TICLv4 used for PID and regression to ONNX format and created a separate inference plugin, TracksterInferenceByCNNv4.
-This PR needs to be tested with the following cms-data PR: cms-data/RecoHGCal-TICL#6.
-Additionally,
this PR should be tested on the .203 workflow :-test parameters:
workflow_opts= -w upgrade
workflow = 29888.203,29688.203
Tagging @felicepantaleo @waredjeb @hatakeyamak