-
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
Updating the WP of the Muon MVA ID #37250
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37250/28852
|
A new Pull Request was created by @andrea21z for master. It involves the following packages:
@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Probably there is some confusion about what a backport means. Please see here: https://cms-sw.github.io/tutorial-resolve-conflicts.html -> "Backporting a PR" for an explanation and clarify in the description if a backport is intended (in any case, a PR against master such as this one is needed). |
test parameters: |
@cmsbuild please test |
Ok, understood. On the other hand, we just found a possible bug and we want to do a couple more checks to be sure before the PR is merged, so I think it's best to stop the PR checks for the moment. @jpata sorry for this ... |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-33d550/23130/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison SummarySummary:
|
Hi @andrea21z , I don't see these changes in this PR, I suppose it is just a copy&paste from the description of #36179. Could you please edit the description, it's misleading. Thank you |
yes, sorry. |
@jpata @clacaputo you can continue with the review, all changes implemented. |
Hello @andrea21z , do you have any plots comparing the old model and the new one? |
Hello @clacaputo, what types of plots would you like to see? The only change with respect to the old model is that we removed a cut in Isolation used to select the muons to train the model. |
Hi @andrea21z , a comparison of the output distribution |
Hi @andrea21z also a reference to a more recent presentation with the new model should be enough. Thanks |
|
Here you can find the latest presentation in the Muon POG meeting, |
Hi @andrea21z , thanks for the link. Could you please confirm me that the new model produces an output more shifted towards 0.9 for prompt muons wrt the old model? I can't get this info from the slides. Here an examples for wf |
Yes, I confirm this is expected. The output distribution of the new model (slide 14) has the maximum around 0.9 and prompt muons have values between 0.7 and 0.9. |
+reconstruction
|
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) |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-33d550/23565/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
+1 |
PR description:
We have updated the Muon MVA ID model and we need to change the cut of the WP Tight, Cuts in IP are also added to the Tight working point.
PR validation:
We execute the basic tests suggested in the CMSSW PR instructions
if this PR is a backport please specify the original PR and why you need to backport that PR: