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

Speedup muon validation #33610

Merged
merged 5 commits into from May 30, 2021
Merged

Conversation

abbiendi
Copy link
Contributor

@abbiendi abbiendi commented May 3, 2021

This PR is mostly aimed at reducing the cpu time needed for the official muon validation sequence.
It restricts the SimToReco association to subsamples of the TrackingParticle (TP) collection, relevant for the muon validation, which are preselected as vectors of references in the Event.

"probeTracks", i.e. inner tracks kinematically compatible with identified muons, as well as "displacedTracks", are tested with respect to TP with ptmin > 2 GeV

Particle Flow muons, are tested w.r.t. general TP with ptmin>0.5 GeV && ptot>2.5 GeV (without muon truth selection)

All the other muon collections (including the hlt muon collections) are tested w.r.t. :

  • TP from muons (pdgId=+/-13) with momentum above minimum thresholds (default setting pt>0.5 GeV && ptot>2.5 GeV)
  • TP having hits in the muon detectors (independent of the momentum)

These definitions seem conservative, could be fine-tuned or the logic could be modified if needed.

In addition a fix is applied to the validation of global muons which now by default requires the track to have at least a valid muon hit (therefore global muons made of only inner tracker hits are considered as fakes). A new parameter in MuonAssociatorByHits is controlling this behaviour (called rejectBadGlobal, default is True. In a few cases it is set to False to allow tracks extending only in the inner tracker, according to the expected algorithm, as for DYT or TuneP).

The result of these changes is that a reconstructed muon can be considered as a fake not only if it does not pass the quality/purity thresholds, but also if there is no found TrackingParticle matching it in the selected subsample.
No changes are produced on the standalone (outer) muons.
(except that the denominator for efficiencies is restricted to the selected TP, so TP which will never reach the muon detectors are generally not counted for the efficiency. This gives a small increase of efficiencies throughout)
Almost no changes of the pure inner tracks (except that due to the better cpu performance I lowered the ptmin of probed tracks to 3 GeV, it was 4 GeV).
Increased fake rates for reco muons (where with this I mean all muon flavours where one expects a matching between a inner track and hits or segments, therefore including Tracker muons, GEM muons etc...), also depending on the chosen settings.
In practice for reco muons the fake rates quantify also the misid rate, not just the rate of imperfect tracking.

PR validation:

The results have been validated w.r.t. the default CMSSW_11_3_0_pre6 on about 1K events of:

In addition basic tests have been done for a number of other workflows: SingleMuon gun, FastSim, Phase2, and premixing, all successful.

Obviously this PR should not enter in release together with significant changes to the muon reco or ID, otherwise a dedicated validation is needed to decouple the effects.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2021

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33610/22451

  • This PR adds an extra 72KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33610/22495

  • This PR adds an extra 56KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2021

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

It involves the following packages:

SimMuon/MCTruth
Validation/RecoMuon

@civanch, @kmaeshima, @andrius-k, @mdhildreth, @ErnestaP, @cmsbuild, @jfernan2, @ahmad3213, @rvenditti can you please review it and eventually sign? Thanks.
@HuguesBrun, @battibass, @jhgoh, @calderona, @sscruz, @Fedespring, @cericeci, @trocino, @slomeo, @rociovilar this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

jfernan2 commented May 5, 2021

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b486f6/14877/summary.html
COMMIT: 18cfcad
CMSSW: CMSSW_12_0_X_2021-05-04-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33610/14877/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b486f6/14877/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2662646
  • DQMHistoTests: Total failures: 5102
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2657521
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.088 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 0.012 KiB MessageLogger/Errors
  • DQMHistoSizes: changed ( 23234.0,... ): 0.012 KiB MessageLogger/Warnings
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: found differences in 4 / 36 workflows

@jfernan2
Copy link
Contributor

jfernan2 commented May 5, 2021

@abbiendi I don't understand why the PR is introducing changes also in Tracking and Vertexing for two wfs apart from those expected in Muons/recoMuonV
https://tinyurl.com/yesh39gl
https://tinyurl.com/yzs47fkg

All the changes here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_0_X_2021-05-04-2300+b486f6/42627/dqm-histo-comparison-summary.html

Thanks

@abbiendi
Copy link
Contributor Author

abbiendi commented May 6, 2021

@jfernan2 please can you tell me how can I access the DQM files directly ? I tried to download one with "wget" but it failed. Browsing with firefox I am not sure what I see. Some histos are missing ? I suspect the compared files could have different statistics

@jfernan2
Copy link
Contributor

jfernan2 commented May 6, 2021

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b486f6/15283/summary.html
COMMIT: 18cfcad
CMSSW: CMSSW_12_0_X_2021-05-24-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33610/15283/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b486f6/15283/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2650486
  • DQMHistoTests: Total failures: 18472
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2631992
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.092 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 0.012 KiB MessageLogger/Errors
  • DQMHistoSizes: changed ( 23234.0,... ): 0.012 KiB MessageLogger/Warnings
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: found differences in 4 / 36 workflows

@abbiendi
Copy link
Contributor Author

Please, if you want me to repeat my test, give me the recipe, I am a bit lost...
We should have a test of IB+anything else necessary without / with this PR
I need to know exactly what is the "anything else necessary" : just #33803 ? or should I change something else ?

@silviodonato
Copy link
Contributor

Please, if you want me to repeat my test, give me the recipe, I am a bit lost...
We should have a test of IB+anything else necessary without / with this PR
I need to know exactly what is the "anything else necessary" : just #33803 ? or should I change something else ?

@abbiendi you can try to compare manually
#33610 (comment) (#33803 + #33610)
vs
#33803 (comment) (#33803)

@abbiendi
Copy link
Contributor Author

Please, if you want me to repeat my test, give me the recipe, I am a bit lost...
We should have a test of IB+anything else necessary without / with this PR
I need to know exactly what is the "anything else necessary" : just #33803 ? or should I change something else ?

@abbiendi you can try to compare manually
#33610 (comment) (#33803 + #33610)
vs
#33803 (comment) (#33803)

done!
I confirm that now both the 2D and the 1D histo from fits are exactly the same.
I referred to the same histograms I was testing before.
( /DQMData/Run 1/Tracking/Run summary/TrackBuilding/jetCoreRegionalStep_quickAssociatorByHits/thetapull_vs_eta (2D)
and h_thetapulleta_Mean (1D) )
So I do not expect other surprises, I would try to merge #33803 and then redo the jenkins

@silviodonato
Copy link
Contributor

@abbiendi thank you for the check!

@silviodonato
Copy link
Contributor

please test

@silviodonato
Copy link
Contributor

#33803 is merged

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b486f6/15404/summary.html
COMMIT: 18cfcad
CMSSW: CMSSW_12_0_X_2021-05-28-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33610/15404/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b486f6/15404/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2650486
  • DQMHistoTests: Total failures: 4042
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2646422
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.092 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 0.012 KiB MessageLogger/Errors
  • DQMHistoSizes: changed ( 23234.0,... ): 0.012 KiB MessageLogger/Warnings
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: found differences in 4 / 36 workflows

@jfernan2
Copy link
Contributor

+1
Thank everybody for the help dealing with this PR!!

@qliphy
Copy link
Contributor

qliphy commented May 30, 2021

+1

@cmsbuild cmsbuild merged commit a34ab38 into cms-sw:master May 30, 2021
@qliphy
Copy link
Contributor

qliphy commented May 31, 2021

With this PR merged, we got IB failure with workflow 1311.0:
https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/relVal/CMSSW_12_0/2021-05-30-2300?selectedArchs=slc7_amd64_gcc900&selectedFlavors=X&selectedStatus=failed
An exception of category 'ProductNotFound' occurred while
[0] Processing Event run: 1 lumi: 35 event: 10207 stream: 0
[1] Running path 'validation_step'
[2] Calling method for module MuonAssociatorEDProducer/'tpToL3GlbMuonAssociation'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for a container with elements of type: reco::Track
Looking for module label: hltIterL3GlbMuon
Looking for productInstanceName:

@abbiendi would you please have a fix soon?

With last IB CMSSW_12_0_X_2021-05-30-0000, it works well.
And then you can reproduce the error as below:
cmsrel CMSSW_12_0_X_2021-05-30-0000
cd CMSSW_12_0_X_2021-05-30-0000/src
git cms-merge-topic 33610
scram b -j8
runTheMatrix.py -l 1311.0

@abbiendi
Copy link
Contributor Author

I'm looking into it

@Martin-Grunewald
Copy link
Contributor

The same problem I have in a few TSG tests!

@abbiendi
Copy link
Contributor Author

abbiendi commented Jun 1, 2021

it's a bit tricky, not yet understood. The broken workflow seems to have a "fake" hlt, and the muon track collections are not there in the Event. So there is nothing to do for the validation. I do not see why the new IB is broken while until CMSSW_12_0_X_2021-05-30-0000 everything worked. The muon hlt collections were absent also there.
@Martin-Grunewald can you be more specific: which problem do you have exactly ? and do you have any clues ?

@mrodozov
Copy link
Contributor

mrodozov commented Jun 1, 2021

I think all three relvals failing started after this (at lest it shows the same missing products)

@Martin-Grunewald
Copy link
Contributor

I meant I see the same problem, ie, same error message! A collection being absent is not allowed to crash the workflow esp. for HLT as the trigger producing the collection may not have been run to completion, so missing products must be expected. So this behaviour seems to have been changed by this PR.

@abbiendi
Copy link
Contributor Author

abbiendi commented Jun 1, 2021

maybe I found the problem

@abbiendi
Copy link
Contributor Author

abbiendi commented Jun 1, 2021

should be ok with PR#33938

@Martin-Grunewald
Copy link
Contributor

Indeed solves the problem in the TSG tests, thanks!

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

10 participants