-
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
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.
Pass as a reference
|
||
double computeMva(const TrajectorySeed&, | ||
GlobalVector, | ||
edm::Handle<l1t::MuonBxCollection>, |
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.
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>, |
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.
same here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for the untracked parameters
} | ||
|
||
edm::ESHandle<TrackerGeometry> trkGeom; | ||
iSetup.get<TrackerDigiGeometryRecord>().get(trkGeom); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
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.
sqrt out of the loop
@wonpoint4 please provide a recipe to verify that the code runs without crashing (at least) |
+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 |
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