-
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
Some clean up on reco muon classes #31789
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31789/19059
|
A new Pull Request was created by @perrotta for master. It involves the following packages: DataFormats/MuonReco @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins.
|
-1 Tested at: 56cd1d6 CMSSW: CMSSW_11_2_X_2020-10-14-1100 I found follow errors while testing this PR Failed tests: Build ClangBuild
I found compilation warning when building: See details on the summary page.
I found compilation warning while trying to compile with clang. Command used:
See details on the summary page. |
Comparison not run due to Build errors (RelVals and Igprof tests were also skipped) |
Indeed, this is exactly the compilation warning that allowed me to pinpoint the issue reported in #31783 I can apply a patch here to reproduce the exact previous behavior without getting that warning (" just check if the size of But maybe it is better to understand the original intentions of the author of the code and apply a more appropriate fix, if needed |
segment != chamber->segmentMatches.end(); | ||
++segment) { | ||
for (auto& chamber : muMatches_) | ||
for (auto& segment : chamber.segmentMatches) { |
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 about:
if (i+chamber.segmentMatches.size()<n ) i+=chamber.segmentMatches.size()) continue;
return chamber.segmentMatches[n-i].t0;
?
for (int detectorIdx = 1; detectorIdx < 3; ++detectorIdx) { | ||
float dist = muon.trackDist(stationIdx, detectorIdx, arbitrationType); | ||
if (dist < maxChamberDist && | ||
dist / muon.trackDistErr(stationIdx, detectorIdx, arbitrationType) < maxChamberDistPull) |
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.
dist < maxCham... * muon.trackDist.....;
avoid a costly div
@@ -564,7 +564,7 @@ void MuonIdProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup) | |||
|
|||
for (auto& muon : *outputMuons) { | |||
if (muon.innerTrack().get() == trackerMuon.innerTrack().get() && | |||
cos(phiOfMuonIneteractionRegion(muon) - phiOfMuonIneteractionRegion(trackerMuon)) > 0) { | |||
cos(phiOfMuonInteractionRegion(muon) - phiOfMuonInteractionRegion(trackerMuon)) > 0) { |
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.
cos(alpha) >0 if |alpha|<pi/2 once alpha is between -pi and pi....
please used dphi function and avoid calling cos.
@@ -665,7 +665,7 @@ void MuonIdProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup) | |||
// predict direction based on the muon interaction region location | |||
// if it's available | |||
if (muon.isStandAloneMuon()) { | |||
if (cos(phiOfMuonIneteractionRegion(muon) - muon.phi()) > 0) { | |||
if (cos(phiOfMuonInteractionRegion(muon) - muon.phi()) > 0) { |
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
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
igprof with the automatic tests keeps failing. By the way, if I run the same workflow 23434.21 myself, I get the very same error on opening the input MB file for the pileup: I suspect there is really something related to that file, or its physical location. However, I run igprof on another PU workflow, 10224, and compared the timing for muonIdProducer without and with this PR. As I anticipated in the PR description, the difference appears to be rather small, at the limit of the significance of the method:
Nevertheless, I would still be in favor of integrating this PR, if there are no opinions against it |
+1 |
Comparison job queued. |
@perrotta , profiling works now |
Comparison is ready Comparison Summary:
|
rpcMatch != chamberMatch->rpcMatches.end(); | ||
rpcMatch++) { | ||
curMask = 1 << ((chamberMatch->station() - 1) + 4 * (rpcIndex - 1)); | ||
// for (auto& rpcMatch : chamberMatch.rpcMatches) { // TO BE FIXED: there is clearly something odd here |
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.
@jhgoh please confirm that what implemented here for the rpc matches (which faithfully reproduces what was coded so far in CMSSW) is actually the sought behaviour, and it did not reveal a bug in the the original code instead.
If it is confirmed being ok, I will remove the comment lines left as a warning.
+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 |
PR description:
Some cleanup is applied in the MuonReco DataFormat, mostly replacing loop expressions with more readable range based ones. This simplifications, among other things, allowed pinpointing the issue described in #31783 (which is not addressed here, waiting for the input from the muon pog)
Moreover, also a few simplifications in the code are implemented, like avoiding repeated computations of the same quantities or reducing the loop iterations when possible. This is not expected to provide significant improvements in the timing, but it shouldn't hurt too.
The updates in the MuonIdProducer are meant to simplify and speed up the algo in a few points, and fix the name of a method which is only used internally to that producer (and therefore does not need to be propagated elsewhere).
PR validation:
I just checked that the code runs without errors in a typical Run2 workflow.
No change is expected in output: therefore, if any shows up from the automatic comparisons it may suggest some bug/typo in this implementation
@trocino @ArnabPurohit FYI