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

Filter tracks extrapolated to the muon system #41323

Merged
merged 8 commits into from
Jun 13, 2023

Conversation

CeliaFernandez
Copy link
Contributor

PR description:

This PR modifies the track extrapolation to the muon chambers to reduce the time taken in muons1stStep sequence. In the current setup all tracks (passing soft pt and eta requirements) are extrapolated to all chambers that are geometrically compatible with the track parameters. The proposed changes allow to check if there are segments/hits in these chambers before initiating any extrapolation. If there are segments/hits, the track is extrapolated to the compatible chambers, if not, extrapolation is aborted.

Implementation was built on top of 13_0_0_pre3.

The time taken by muon1stStep was reduced from 4.5% to 3.8%, which translates into a reduction of total RecoMuon time from 7.0% to 6.1%. This measurement was done in lxplus with low precision, so checks are required.

No changes are expected in any collection as this PR is not intended to modify any physics behavior but reduce the time taken by muon reconstruction.

More info in this presentation: https://indico.cern.ch/event/1257332/

Some notes:

  1. ME0 segments were loaded by default, so ME0 hits were also loaded as well. There is an ongoing migration from GEM → ME0 ( #31424), should we take advantage here to remove them from TrackAssociator?
  2. There is a merge conflict with changes introduced by other PR (Add more fillDescriptions to Alignment/OfflineValidation plugins #40698), although the proposed changes should give an equivalent outcome. How can I proceed here?

PR validation:

This PR was tested with standard 11834.21 workflow. It passes the runTheMatrix tests with the exception of 2 where the connection of the input samples couldn't be readed.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

This PR is not a backport.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41323/35143

  • This PR adds an extra 44KB to repository

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41323/35145

  • This PR adds an extra 44KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @CeliaFernandez (Celia Fernández Madrazo) for master.

It involves the following packages:

  • RecoEcal/EgammaClusterProducers (reconstruction)
  • RecoMET/METAlgorithms (reconstruction)
  • RecoMuon/MuonIdentification (reconstruction)
  • TrackingTools/TrackAssociator (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@gouskos, @VourMa, @felicepantaleo, @jainshilpi, @abbiendi, @CeliaFernandez, @argiro, @a-kapoor, @thomreis, @Prasant1993, @varuns23, @seemasharmafnal, @mmarionncern, @missirol, @ahinzmann, @andrea21z, @JanFSchulte, @jhgoh, @lgray, @jdolen, @HuguesBrun, @cericeci, @trocino, @rociovilar, @Sam-Harper, @wang0jin, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @bellan, @nhanvtran, @gkasieczka, @schoef, @ebrondol, @mtosi, @dgulhan, @rchatter, @valsdav, @Fedespring, @sobhatta, @afiqaize, @gpetruc, @wrtabb, @mariadalfonso, @sameasy, @mmusich, @ram1123 this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41323/35150

  • This PR adds an extra 52KB to repository

@cmsbuild
Copy link
Contributor

Pull request #41323 was updated. @cmsbuild, @mandrenguyen, @clacaputo can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Apr 12, 2023

enable profiling

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-619298/31955/summary.html
COMMIT: dcf6752
CMSSW: CMSSW_13_1_X_2023-04-12-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41323/31955/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 3 lines to the logs
  • Reco comparison results: 187 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3459609
  • DQMHistoTests: Total failures: 67
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3459520
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor

mmusich commented Apr 12, 2023

No changes are expected in any collection as this PR is not intended to modify any physics behavior but reduce the time taken by muon reconstruction.

Looks like this PR is causing genuine changes: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_13_1_X_2023-04-12-1100+619298/56509/validateJR.html

@CeliaFernandez
Copy link
Contributor Author

No changes are expected in any collection as this PR is not intended to modify any physics behavior but reduce the time taken by muon reconstruction.

Looks like this PR is causing genuine changes: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_13_1_X_2023-04-12-1100+619298/56509/validateJR.html

Thanks @mmusich , I did not observe these changes before... Indeed the proposed changes are not fully transparent and this effects need to be understood.
I see that the number of muons remains the same but the segment information has changed for some of them. From a quick look to the matched segment size plots it seems that the variation is happening in muons where the matches (segments/hits) are always zero e.g.

And at the same time, the number of chambers seems to be "switching" some of the values to zero:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_13_1_X_2023-04-12-1100+619298/56509/validateJR/9.0_Higgs200ChargedTaus/all_RECO_step3/c_recoMuons_muonsFromCosmics__RECO_obj_numberOfChambers.png

I suspect that the muons suffering these changes are those whose tracks are successfully extrapolated to chambers but none of the extrapolated chambers have segments. These were counted before when all the extrapolations were done. However here, where the extrapolations are done only if at least one of the chambers have segments, these are not counted. Since tracker muons have at least one segment (the implementation was done taking this into account) I suspect that the affected muons are global/standalone muons which do not make it to tracker muons.

I will do more studies to check is this is the case and try to fully understand what is happening...

@smuzaffar smuzaffar modified the milestones: CMSSW_13_1_X, CMSSW_13_2_X May 4, 2023
@mandrenguyen
Copy link
Contributor

@CeliaFernandez Any update here? Would you say these differences are now expected?

@CeliaFernandez
Copy link
Contributor Author

Hi @mandrenguyen, sorry for the late reply. It took me more time to check than I thought... After doing so, the hypothesis presented here is confirmed:

I suspect that the muons suffering these changes are those whose tracks are successfully extrapolated to chambers but none of the extrapolated chambers have segments. These were counted before when all the extrapolations were done. However here, where the extrapolations are done only if at least one of the chambers have segments, these are not counted. Since tracker muons have at least one segment (the implementation was done taking this into account) I suspect that the affected muons are global/standalone muons which do not make it to tracker muons.

Whenever there are no segments/hits, the track extrapolation is aborted and numberOfChambers() is set to zero. This leads to changes in this variable for muons that are not reconstructed as TrackerMuons i.e. Standalone muons (which could be Global o not).
This is not expected to have any impact on physics, and no change in efficiencies or IDs has been observed. Changes are expected and we can go on with them.

@clacaputo
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-619298/33094/summary.html
COMMIT: dcf6752
CMSSW: CMSSW_13_2_X_2023-06-12-1100/el8_amd64_gcc11
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41323/33094/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 9 lines from the logs
  • Reco comparison results: 117 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3190910
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3190882
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mandrenguyen
Copy link
Contributor

+reconstruction
Changes are in line with expectations as described in #41323 (comment)
Didn't occur to me at the ORP, but would be nice to squeeze this into 13_2_0_pre2

@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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

type performance-improvements

@mandrenguyen
Copy link
Contributor

type muon

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 0717bc8 into cms-sw:master Jun 13, 2023
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.

8 participants