modernize `TkAlCaRecoMonitor` source and configuration #40877

mmusich · 2023-02-24T16:03:17Z

PR description:

Title says it all:

add a fillDescriptions;
use range-based loops;
reduce code duplication.

PR validation:

cmssw compiles;
scram b runtests_testDiMuonVertexMonitor use-ibeos passes.

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:

N/A

cmsbuild · 2023-02-24T16:09:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40877/34347

This PR adds an extra 16KB to repository

cmsbuild · 2023-02-24T16:09:41Z

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

DQMOffline/Alignment (dqm)

@emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please review it and eventually sign? Thanks.
@rociovilar 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

mmusich · 2023-02-24T18:00:32Z

please test

cmsbuild · 2023-02-24T19:15:52Z

-1

Failed Tests: UnitTests RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15e640/30892/summary.html
COMMIT: 6c36d3e
CMSSW: CMSSW_13_1_X_2023-02-24-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40877/30892/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test testDiMuonVertexMonitor had ERRORS

RelVals

138.4138.4_PromptCollisions2021/step2_PromptCollisions2021.log
138.5138.5_ExpressCollisions2021/step2_ExpressCollisions2021.log
140.53140.53_RunHI2011/step2_RunHI2011.log

Expand to see more relval errors ...

RelVals-INPUT

4.64.6_MinimumBias2010A/step2_MinimumBias2010A.log
138.4138.4_PromptCollisions2021/step2_PromptCollisions2021.log
138.5138.5_ExpressCollisions2021/step2_ExpressCollisions2021.log

Expand to see more relval errors ...

cmsbuild · 2023-02-24T20:16:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40877/34351

This PR adds an extra 16KB to repository

cmsbuild · 2023-02-24T20:17:10Z

Pull request #40877 was updated. @emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please check and sign again.

mmusich · 2023-02-24T20:17:41Z

@cmsbuild, please test

mmusich · 2023-02-24T20:55:44Z

please abort

cmsbuild · 2023-02-24T22:40:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40877/34352

This PR adds an extra 16KB to repository

cmsbuild · 2023-02-24T22:41:13Z

Pull request #40877 was updated. @emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please check and sign again.

mmusich · 2023-02-24T22:54:11Z

please test

cmsbuild · 2023-02-25T03:49:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15e640/30898/summary.html
COMMIT: 4c5640c
CMSSW: CMSSW_13_1_X_2023-02-24-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40877/30898/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 5 lines to the logs
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3528955
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3528927
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

emanueleusai · 2023-02-27T00:58:40Z

+1

cmsbuild · 2023-02-27T00:59:03Z

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)

perrotta

These minor points could be left for a follow up update, if deemed worth to

perrotta · 2023-02-27T09:19:58Z

DQMOffline/Alignment/src/TkAlCaRecoMonitor.cc

-        if ((*itJet).pt() > maxJetPt_ && dR < minJetDeltaR)
+      for (const auto &itJet : *jets) {
+        jetPt_->Fill(itJet.pt());
+        dR = deltaR(track, itJet);


Minor point: deltaR2 is computationally less expensive

but then isn't taking the root moving the expense elsewhere (notice we are interested in dR, not dR2). Or are you suggesting to plot directly dR2?

As I said, it is a minor point. But yes, computing the square root only once before filling the histograms is less expensive than having to compute it at every jet or track

sorry, I admit I misunderstood what you proposed. Here's the requested improvement: #40887.

perrotta · 2023-02-27T09:20:04Z

DQMOffline/Alignment/src/TkAlCaRecoMonitor.cc

-         ++track2) {
-      dR = deltaR((*track), (*track2));
+    for (const auto &track2 : *trackCollection) {
+      dR = deltaR(track, track2);


Minor point: deltaR2 is computationally less expensive

perrotta · 2023-02-27T09:28:42Z

DQMOffline/Alignment/interface/TkAlCaRecoMonitor.h

  void bookHistograms(DQMStore::IBooker &, edm::Run const &, edm::EventSetup const &) override;
  void analyze(const edm::Event &, const edm::EventSetup &) override;

 private:
+  static constexpr const double kMuonMass_ = 0.10565836;


Ultra minor point: it is funny, there are slightly different values of the muon mass scattered in our code. The PDG value is 0.1056583755, tinily larger than what written here, Lacking a centrally accessible value of it in CMSSW, one could stick on the PDG value

I would be really glad if we actually had to care about the sub per million differences in the muon mass discrepancy in the di-muon resonance mass that we are monitoring. We have much worse problems than this :D

In any case done at #40887.

perrotta · 2023-02-27T09:31:48Z

+1

cmsbuild added this to the CMSSW_13_1_X milestone Feb 24, 2023

cmsbuild added dqm-pending pending-signatures tests-pending orp-pending code-checks-pending labels Feb 24, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 24, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 24, 2023

cmsbuild added tests-rejected and removed tests-started labels Feb 24, 2023

mmusich force-pushed the modernizeTkAlCaRecoMonitor branch from 6c36d3e to 50f31c3 Compare February 24, 2023 20:08

cmsbuild added tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Feb 24, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 24, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 24, 2023

cmsbuild removed the tests-started label Feb 24, 2023

cmsbuild added the tests-pending label Feb 24, 2023

modernize TkAlCaRecoMonitor source and configuration

4c5640c

mmusich force-pushed the modernizeTkAlCaRecoMonitor branch from 50f31c3 to 4c5640c Compare February 24, 2023 22:32

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 24, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 24, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 24, 2023

cmsbuild added tests-approved and removed tests-started labels Feb 25, 2023

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Feb 27, 2023

perrotta reviewed Feb 27, 2023

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Feb 27, 2023

cmsbuild merged commit 8377d6a into cms-sw:master Feb 27, 2023

mmusich mentioned this pull request Feb 27, 2023

Improve performance of TkAlCaRecoMonitor #40887

Merged

This was referenced Feb 27, 2023

Custom Run 3 PFScouting NanoAOD #40438

Merged

[BuildRules] Enable Alpaka/Rocm backend cms-sw/cmsdist#8301

Closed

mmusich deleted the modernizeTkAlCaRecoMonitor branch March 6, 2023 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modernize `TkAlCaRecoMonitor` source and configuration #40877

modernize `TkAlCaRecoMonitor` source and configuration #40877

mmusich commented Feb 24, 2023 •

edited

Loading

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

mmusich commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

mmusich commented Feb 24, 2023

mmusich commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

mmusich commented Feb 24, 2023

cmsbuild commented Feb 25, 2023

emanueleusai commented Feb 27, 2023

cmsbuild commented Feb 27, 2023

perrotta left a comment

perrotta Feb 27, 2023

mmusich Feb 27, 2023

perrotta Feb 27, 2023

mmusich Feb 27, 2023

perrotta Feb 27, 2023

perrotta Feb 27, 2023

mmusich Feb 27, 2023

mmusich Feb 27, 2023

perrotta commented Feb 27, 2023

modernize TkAlCaRecoMonitor source and configuration #40877

modernize TkAlCaRecoMonitor source and configuration #40877

Conversation

mmusich commented Feb 24, 2023 • edited Loading

PR description:

PR validation:

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:

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

mmusich commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

Unit Tests

RelVals

RelVals-INPUT

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

mmusich commented Feb 24, 2023

mmusich commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

mmusich commented Feb 24, 2023

cmsbuild commented Feb 25, 2023

Comparison Summary

emanueleusai commented Feb 27, 2023

cmsbuild commented Feb 27, 2023

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented Feb 27, 2023

modernize `TkAlCaRecoMonitor` source and configuration #40877

modernize `TkAlCaRecoMonitor` source and configuration #40877

mmusich commented Feb 24, 2023 •

edited

Loading