MuonHLTSeed MVA Classifier 120X #33983
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33983/23087
|
|
A new Pull Request was created by @wonpoint4 (Won Jun) for master. It involves the following packages: RecoMuon/TrackerSeedGenerator @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
assign hlt |
|
New categories assigned: hlt @fwyzard,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-84937d/15721/summary.html CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-84937d/15721/llvm-analysis/cmsclangtidy.txt for details. Comparison SummarySummary:
|
|
@fwyzard,@Martin-Grunewald Could you review this PR? |
| desc.addOptionalUntracked<std::vector<double>>("mvaScaleMeanE", {0.}); | ||
| desc.addOptionalUntracked<std::vector<double>>("mvaScaleStdE", {1.}); | ||
|
|
||
| descriptions.add("MuonHLTSeedMVAClassifier", desc); |
There was a problem hiding this comment.
Why are several parameters declared Optional Untracked? In case they affect physics, they must be non-optional and also tracked. Optional is not recognised by ConfDB.
There was a problem hiding this comment.
Thank you for the review.
If I change them into desc.add from desc.addOptionalUntracked, I got these messages when I do edmPluginHelp -p .
How can I resolve this issue? These xml files are our model files, so they can be modified if I re-train models with new MC samples.
shell
[wjun@lxplus724 src]$ edmPluginHelp -p MuonHLTSeedMVAClassifier
1 MuonHLTSeedMVAClassifier (stream::EDProducer) "pluginRecoMuonTrackerSeedGeneratorPlugins.so"
START ERROR FROM edmPluginHelp
The executable "edmPluginHelp" encountered a problem while filling a
ParameterSetDescription. We give up for this plugin and skip printing out
this description and any following descriptions for this plugin. Here
is the info from the exception:
An exception of category 'FileInPathError' occurred.
Exception Message:
edm::FileInPath unable to find file RecoMuon/TrackerSeedGenerator/data/Run3v6_Barrel_hltIter2.xml anywhere in the search path.
The search path is defined by: CMSSW_SEARCH_PATH
${CMSSW_SEARCH_PATH} is: /afs/cern.ch/user/w/wjun/public/CMSSW_12_0_X_2021-06-04-1100/poison:/afs/cern.ch/user/w/wjun/public/CMSSW_12_0_X_2021-06-04-1100/src:/afs/cern.ch/user/w/wjun/public/CMSSW_12_0_X_2021-06-04-1100/external/slc7_amd64_gcc900/data:/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc900/cms/cmssw-patch/CMSSW_12_0_X_2021-06-04-1100/poison:/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc900/cms/cmssw-patch/CMSSW_12_0_X_2021-06-04-1100/src:/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc900/cms/cmssw-patch/CMSSW_12_0_X_2021-06-04-1100/external/slc7_amd64_gcc900/data
Current directory is: /afs/cern.ch/user/w/wjun/public/CMSSW_12_0_X_2021-06-04-1100/src
END ERROR FROM edmPluginHelp
There was a problem hiding this comment.
You must add the data file also as external, to be run together with this PR. Please prepare a cmsData PR with it
| std::vector<double> scale_std_; | ||
|
|
||
| double computeMva(const TrajectorySeed&, | ||
| GlobalVector, |
|
|
||
| double computeMva(const TrajectorySeed&, | ||
| GlobalVector, | ||
| edm::Handle<l1t::MuonBxCollection>, |
There was a problem hiding this comment.
Do you really need to pass the handle? Shouldn't the (reference to the) MuonBxCollection work?
| GlobalVector, | ||
| edm::Handle<l1t::MuonBxCollection>, | ||
| int minL1Qual, | ||
| edm::Handle<reco::RecoChargedCandidateCollection>, |
| std::unique_ptr<const GBRForest> gbrForest_; | ||
|
|
||
| void getL1MuonVariables( | ||
| const TrajectorySeed&, GlobalVector, edm::Handle<l1t::MuonBxCollection>, int minL1Qual, float&, float&) const; |
There was a problem hiding this comment.
GlobalVector and MuonBxCollection passed by reference
| void getL1MuonVariables( | ||
| const TrajectorySeed&, GlobalVector, edm::Handle<l1t::MuonBxCollection>, int minL1Qual, float&, float&) const; | ||
| void getL2MuonVariables( | ||
| const TrajectorySeed&, GlobalVector, edm::Handle<reco::RecoChargedCandidateCollection>, float&, float&) const; |
There was a problem hiding this comment.
GlobalVector and RecoChargedCandidateCollection passed by reference
| minL1Qual_(iConfig.getParameter<int>("minL1Qual")), | ||
| baseScore_(iConfig.getParameter<double>("baseScore")) { | ||
| if (!rejectAll_) { | ||
| mvaFileB_ = iConfig.getUntrackedParameter<edm::FileInPath>("mvaFileB"); |
There was a problem hiding this comment.
Ditto for the untracked parameters
| } | ||
|
|
||
| edm::ESHandle<TrackerGeometry> trkGeom; | ||
| iSetup.get<TrackerDigiGeometryRecord>().get(trkGeom); |
There was a problem hiding this comment.
Direct call of function EventSetupRecord::get(ESHandle&) is deprecated and should be replaced with a call to EventSetup::getHandle(ESGetToken&). To use ESGetToken see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#In_ED_module To get data with the token see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Getting_data_from_EventSetup_wit
| double mva = getSeedMva(mvaEstimator_, seed, global_p, h_L1Muon, minL1Qual_, h_L2Muon, isFromL1_, baseScore_); | ||
|
|
||
| double score = 1. / (1. + std::exp(-1. * mva)); | ||
| bool passMva = ((isB && (score > mvaCutB_)) || (!isB && (score > mvaCutE_))); |
There was a problem hiding this comment.
This is probably more compact and intuitive:
| bool passMva = ((isB && (score > mvaCutB_)) || (!isB && (score > mvaCutE_))); | |
| bool passMva = isB? score > mvaCutB_ : score > mvaCutE_; |
| float dR2tmp = reco::deltaR2(ref_L1Mu->etaAtVtx(), ref_L1Mu->phiAtVtx(), global_p.eta(), global_p.phi()); | ||
|
|
||
| if (dR2tmp < dRdRL1SeedP * dRdRL1SeedP) { | ||
| dRdRL1SeedP = std::sqrt(dR2tmp); |
There was a problem hiding this comment.
Just save dR2tmp, and make the square root only once out of the loop
| float dR2tmp = reco::deltaR2(*ref_L2Mu, global_p); | ||
|
|
||
| if (dR2tmp < dRdRL2SeedP * dRdRL2SeedP) { | ||
| dRdRL2SeedP = std::sqrt(dR2tmp); |
|
@wonpoint4 please provide a recipe to verify that the code runs without crashing (at least) |
cmsbuild
added
code-checks-pending
and removed
code-checks-approved
tests-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33983/23430
|
|
please test |
I confirm. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-84937d/16158/summary.html Comparison SummarySummary:
|
|
+1
|
|
+1 |
cmsbuild
added
fully-signed
hlt-approved
and removed
hlt-pending
pending-signatures
labels
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
BackPort(#33983) MuonHLTSeed MVA Classifier to 113X
PR description:
This PR adds a new class for a MVA based TrajectorySeed quality estimator and a new EDProducer for selecting high quality seeds for the Run 3 Muon HLT development.
The PR has no impact on any existing modules or codes.
The work has been presented and approved by the Muon POG.
The latest presentation given at Muon POG (HLT+RECO) meeting can be found in here: https://indico.cern.ch/event/1012931/#6-update-on-inside-out-seeding
PR validation:
Ran MuonHLT workflow