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
OMTF muon trigger emulator: fix in the RPC clusterization #37866
OMTF muon trigger emulator: fix in the RPC clusterization #37866
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37866/29813
|
A new Pull Request was created by @kbunkow for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Please test |
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
Please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cadc6f/24564/summary.html Comparison SummarySummary:
|
+l1 |
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) |
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 just revert the change in L1Trigger/L1TMuonOverlapPhase1/plugins/L1TMuonOverlapPhase1TrackProducer.cc
Then it can be merged
@@ -1,4 +1,5 @@ | |||
#include "L1TMuonOverlapPhase1TrackProducer.h" | |||
#include "L1Trigger/L1TMuonOverlapPhase1/plugins/L1TMuonOverlapPhase1TrackProducer.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.
please revert: it is in the same \plugin area
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.
OK, will be done in a moment.
In the two other plugins in the package, the include of the plugin header also contains the path, should it be corrected there as well - for consistency?
cmssw/L1Trigger/L1TMuonOverlapPhase1/plugins/L1MuonOverlapPhase1ParamsDBProducer.cc
Line 1 in 023fd65
#include "L1Trigger/L1TMuonOverlapPhase1/plugins/L1MuonOverlapPhase1ParamsDBProducer.h" |
cmssw/L1Trigger/L1TMuonOverlapPhase1/plugins/L1TMuonOverlapPhase1ParamsESProducer.cc
Line 1 in 023fd65
#include "L1Trigger/L1TMuonOverlapPhase1/plugins/L1TMuonOverlapPhase1ParamsESProducer.h" |
Karol
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.
That would be great, thank you!
(Even better would be to merge the .h and .cc files into one single .cc, but of course this can also be deferred to some possinle later cleanup of the code)
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 just amended the original commit, the "L1Trigger/L1TMuonOverlapPhase1/plugins/" in the header include directive was removed in L1MuonOverlapPhase1ParamsDBProducer.cc and L1TMuonOverlapPhase1ParamsESProducer.cc, and is not added in the L1TMuonOverlapPhase1TrackProducer.cc.
Karol
Small fixes that should improve data-to-emulator agreement for the OMTF muon trigger: RpcClusterization::getClusters: sort() called in all cases OMTFinputMaker.cc RpcDigiToStubsConverterOmtf::addRPCstub: fix in setting MuonStub::RPC_DROPPED L1MuonOverlapPhase1ParamsDBProducer.cc and L1TMuonOverlapPhase1ParamsESProducer.cc: removed "L1Trigger/L1TMuonOverlapPhase1/plugins/" from the header include directive
e4b2efc
to
4728f3c
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37866/29931
|
Pull request #37866 was updated. @epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cadc6f/24622/summary.html Comparison SummarySummary:
|
+l1 |
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 |
PR description:
Small fixes that should improve data-to-emulator agreement for the OMTF muon trigger, are relevant for the OMTF firmware that is deployed since November 2021 and will be used during the RUN3. The fix is in the codes emulating the RPC clusterization, it corrects the handling of some corner cases.
With this fix, the data-to-emulator agreement should improve by about 1% and be better than 99%.
Rather urgent - would be good to have it soon in the DQM (for the first collisions).
Modified files:
RpcClusterization::getClusters: sort() called in all cases
OMTFinputMaker.cc RpcDigiToStubsConverterOmtf::addRPCstub: fix in setting MuonStub::RPC_DROPPED
PR validation:
The data-to-emulator agreement was checked running a private instance of the DQM for a cosmic run from the February CRAFT, the data-to-emulator agreement improved by ~1%.
if this PR is a backport please specify the original PR and why you need to backport that PR:
it is not a backport.