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
L2 tau identification with a CNN #35640
L2 tau identification with a CNN #35640
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35640/25913 |
A new Pull Request was created by @azotz for master. It involves the following packages:
@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Hi, a few comments/questions from a very first look:
|
On a technical note, please squash the commits into one, to tidy up the history. |
@@ -14,5 +14,7 @@ | |||
<use name="DataFormats/HLTReco"/> | |||
<use name="HLTrigger/HLTcore"/> | |||
<use name="RecoTracker/TkTrackingRegions"/> | |||
<use name="cuda"/> |
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.
Are the classes in this package really using cuda
code (I could not see it)?
Otoh, I was expecting the addition of (judging from the include
s in some of the classes):
<use name="CUDADataFormats/SiPixelCluster"/>
<use name="CUDADataFormats/Track"/>
<use name="CUDADataFormats/Vertex"/>
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.
@kandrosov @valeriadamante could you comment this, please?
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 @missirol
If we do not include <use name="cuda"/>
the following error raises:
>> Compiling edm plugin /afs/cern.ch/work/v/vdamante/public/CMSSW_12_1_0_pre3/src/RecoTauTag/HLTProducers/src/VertexFromTrackProducer.cc
In file included from /cvmfs/cms.cern.ch/slc7_amd64_gcc900/cms/cmssw/CMSSW_12_1_0_pre3/src/RecoPixelVertexing/PixelTrackFitting/interface/FitUtils.h:6,
from /afs/cern.ch/work/v/vdamante/public/CMSSW_12_1_0_pre3/src/RecoTauTag/HLTProducers/src/L2TauTagNNProducer.cc:40:
/cvmfs/cms.cern.ch/slc7_amd64_gcc900/cms/cmssw/CMSSW_12_1_0_pre3/src/RecoPixelVertexing/PixelTrackFitting/interface/FitResult.h:7:10: fatal error: cuda_runtime.h: No such file or directory
7 | #include <cuda_runtime.h>
| ^~~~~~~~~~~~~~~~
compilation terminated.
gmake: *** [config/SCRAM/GMake/Makefile.rules:1700: tmp/slc7_amd64_gcc900/src/RecoTauTag/HLTProducers/src/RecoTauTagHLTProducers/L2TauTagNNProducer.cc.o] Error 1
gmake: *** Waiting for unfinished jobs....
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2
I can try to remove <use name="cuda"/>
and include the lines you suggested.
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.
@missirol
I removed <use name="cuda"/>
and I included the lines you suggested and it works.
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 will implement the changes then as suggested
desc.add<edm::InputTag>("L1TauSrc", edm::InputTag("")) | ||
->setComment("Which trigger should the L1 Taus collection pass"); | ||
desc.add<edm::InputTag>("L2Outcomes", edm::InputTag(""))->setComment("L2 CNN outcomes"); | ||
desc.add<double>("DiscrWP", 0.12267940863785043)->setComment("value of discriminator threshold"); |
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 round this up, e.g. 0.123
.
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 @missirol
For the moment I wonder if can we round at 0.1227. Further studies on the rounding will be done to see if we can leave 3 floating or we need other ones.
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, that's okay.
The tests as described in the PR validation all passed. |
The plan is to provide them through |
The instructions in the README are not supposed to replace a unit test, once these developments reach integration phase. Is a unit test necessary at this stage? A shell script could be provided, which follows the instructions in the README |
40a1f39
to
5e1feb7
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35640/25949 |
I rebased the development branch to the recent IB. The squash is incoming. The comments are being addressed by the original authors. |
Pull request #35640 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
5e1feb7
to
1cf90b2
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35640/25952
|
Pull request #35640 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35640/26113
|
Pull request #35640 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
please test Thanks to Andrea for helping with the review. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d40177/19798/summary.html Comparison SummarySummary:
|
+hlt note: cms-data/RecoTauTag-TrainingFiles#7 should be merged before this PR |
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 |
For me, the unit test in question, https://github.com/cms-sw/cmssw/blob/master/RecoTauTag/HLTProducers/test/testL2TauTagNN.py seems unacceptable as a unit test as it appears to run the menu itself in a special environment, customised, which now fails as the input menu has changed. This goes way beyond a supposedly simple unit test. So the required course of action is to remove the unit test or scale it down to become a proper unit test. |
PR description:
In this PR the implementation of a machine learning based approach for the L2 taus is introduced. The chosen ML algorithm is a CNN trained with different hadronic taus production mechanisms (and reweighed in order to avoid overtraining for specific production mechanisms) versus fake taus coming from QCD.
You can find:
The trained CNN and the normalization file (needed to normalize the matrix which will be given in input to the CNN) is currently in the private fork [2] of the RecoTauTag-TrainingFiles repository as you can see from the instructions in the md file. The training files are planned to be provided via
cms-sw/cms-data
, see [3].[1] https://indico.cern.ch/event/1040952/contributions/4401915/attachments/2260841/3837378/L2TauIDCNN.pdf
[2] https://github.com/valeriadamante/RecoTauTag-TrainingFiles/tree/L2Taus
[3] cms-data/RecoTauTag-TrainingFiles#7
PR validation:
The PR has been validated on top of 12_1_0_pre4. The provided test has been validated and runs smoothly.
The following tests were run and passed as expected, since this PR does not include any change to the official workflows:
if this PR is a backport please specify the original PR and why you need to backport that PR:
This PR is not a backport.