Skip to content
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

[muon id] bug fix for high pt muon algorithm ("Picky") #22422

Merged
merged 1 commit into from Mar 12, 2018

Conversation

drkovalskyi
Copy link
Contributor

From @rradogna:

the fix is for the piece of logic that tries to figure out whether the muon fit is using segments or hits - because picky counts hits in individual layers when it looks for showers, so if the fit is using segments it has to get their component rechits to look at individual layers. For some reason it was checking the size() of the rechit to see if it is a hit (size ==1) or segment (size>1). And it failed for 4d segments that have only one phi/theta projection - then size()==1 and the code thinks it is a segment. So I changed this to checking the hit dimension which makes much more sense.

FYI @ptraczyk

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

A new Pull Request was created by @drkovalskyi for master.

It involves the following packages:

RecoMuon/GlobalTrackingTools

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@bellan, @abbiendi, @jhgoh, @echapon, @calderona, @HuguesBrun, @folguera, @battibass, @trocino, @amagitte, @bachtis, @rociovilar this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Mar 2, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26450/console Started: 2018/03/02 17:05

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 2, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22422/26450/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2479021
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2478844
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.23999999996 KiB( 23 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@drkovalskyi drkovalskyi changed the title bug fix for high pt muon algorithm ("Picky") [muon id] bug fix for high pt muon algorithm ("Picky") Mar 7, 2018
@perrotta
Copy link
Contributor

perrotta commented Mar 8, 2018

@drkovalskyi @rradogna
There are no changes in the jenkins outputs: shouldn't we expect some?
Did you study the effect of the fix? Maye a larger stat than the automatic tests is needed to appreciate it?

@drkovalskyi
Copy link
Contributor Author

I would not expect it to be visible easily. The algorithm is relevant only at high pt. Raffaella is not available at the moment. Maybe Piotr can comment (@ptraczyk)

@perrotta
Copy link
Contributor

Tested with 800 events SingleMuPT1000 with 2018 config (wf 10809, 1600 muons at 1 TeV pt).

Two muons get their pt modified with the picky algo (and the new pt is closer to 1 TeV than before, whatever it means with such a low stat):
all_testpr22422vsorig_singlemupt1000in2018wf10809p0c_log10recotracks_tevmuons_picky_reco_obj_pt

The number of picky muons whose chi2 has been modified (typically reduced) is a bit larger: 5 muons (out of 1600):
all_testpr22422vsorig_singlemupt1000in2018wf10809p0c_minrecotracks_tevmuons_picky_reco_obj_chi2 99

Touching the chi2 (and recHit multiplicities, etc.) leads to some other reco::muon (besides the two whose picky muon pt got modified) whose momentum assignment gets modified:
all_testpr22422vsorig_singlemupt1000in2018wf10809p0c_recomuons_muons__reco_obj_pt
(in this plot the pt is cut at 1 TeV, which is rather unfortunate here: this doesn't want and can't be a complete validation of the fix, though, and the plot as a whole just testify that there are more reco::muon affected than than the two muons which got a different momentum measured by the picky algo)

@perrotta
Copy link
Contributor

+1

  • The changes in the code are in line with what declared
  • The effect of the fix is limited to a small fraction of the high pt muons: it can propagate to other derived quantities (e,g, jets, MET, etc.) in case the muon momentum assignment is also affected
  • At a first glance, those changes go in the correct direction, although some larger stat comparison than the limited one inspected in [muon id] bug fix for high pt muon algorithm ("Picky") #22422 (comment) is needed for a complete validation

@cmsbuild
Copy link
Contributor

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 0a3c558 into cms-sw:master Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants