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

Improvements for Tracker Alignment DiMuonValidation #44994

Merged
merged 6 commits into from
May 21, 2024

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented May 17, 2024

PR description:

The goal of this PR is to inject in release few improvements for Tracker Alignment DiMuonValidation setup.
These are:

  • adding 3D histograms of di-muon mass vs η, φ for derivation of 2D bias maps: b4be42b
  • improve the titles of some of the monitoring histograms in bins of muon pseudo-rapidity: 795c78c , 682c03d
  • finally, introduce a new C++ macro to perform fits of the mass bias in bins of track η, φ (commits 5ad5c28, 4987468, 4dcf990) .
    This is foreseen in the longer term to replace the existing Zmumumerge.cc class which is currently used by the so-called "all-in-one" tool. The new plotting script fixes several issues with the final plotting currently provided by the all-in-one; however, the replacement is not done in this PR and will have to be done at a second time.

PR validation:

Private validation was carried out using this branch and operating the new plotting macros.
The plots at this link were obtained. . For a comparison with the output of the current code, see please here.

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:

Not a backport, will be backported to 14.0.X

Cc: @henriettepetersen @TomasKello

@cmsbuild
Copy link
Contributor

cmsbuild commented May 17, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44994/40275

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Alignment/OfflineValidation (alca)

@perrotta, @cmsbuild, @saumyaphor4252, @consuegs can you please review it and eventually sign? Thanks.
@tlampen, @tocheng, @rsreds, @mmusich, @yuanchao, @adewit this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela 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-44994/40282

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

Pull request #44994 was updated. @cmsbuild, @perrotta, @consuegs, @saumyaphor4252 can you please check and sign again.

@mmusich
Copy link
Contributor Author

mmusich commented May 17, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-320b41/39424/summary.html
COMMIT: 4dcf990
CMSSW: CMSSW_14_1_X_2024-05-17-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44994/39424/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test test-das-selected-lumis had ERRORS

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3338976
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3338950
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor Author

mmusich commented May 19, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-320b41/39430/summary.html
COMMIT: 4dcf990
CMSSW: CMSSW_14_1_X_2024-05-19-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44994/39430/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@perrotta
Copy link
Contributor

I am looking at the plots you posted from the private validation. Out of curiosity, could you explain what originates the differences observed between prompt and mp3747 (by the way: what does it mean?)

@mmusich
Copy link
Contributor Author

mmusich commented May 20, 2024

@perrotta

Out of curiosity, could you explain what originates the differences observed between prompt and mp3747 (by the way: what does it mean?)

mp3747 is the internal MillePede numbering identifier for a manual alignment campaign (the 3747-th produced using the MillePede Production System or mps for short).
According to the internal record it's:

Dir       User        Date       CMSSW-version                     Data             Important settings
--------- ----------- ---------- --------------------------------- ---------------- -----------------------------------------------------
mp3747    hpeterse    2023-09-18 CMSSW_13_2_3 (mod.)               data             clone of mp3746 and use as SG, without CRUZET and with zmumu

basically it's an early prototype version of an alignment for the 2023 re-reco.

@perrotta
Copy link
Contributor

Thank you @mmusich
And why the phases are so different between prompt and mp3747? For example:
image
image

@mmusich
Copy link
Contributor Author

mmusich commented May 20, 2024

And why the phases are so different between prompt and mp3747?

I can't tell for sure, I guess different track topologies used in Prompt reco (mainly plain charged hadrons) vs ReReco (adding cosmics and muons from resonances) are affecting systematic deformations in different ways. The goal of the rereco is of course to minimize the amplitude of such deformations. I would not pay too much attention to the actual results shown in the link, it's just a semirandom combination I chose to illustrate the features introduced in this PR. Speaking of which what has all this discussion have to do with the PR itself?

@perrotta
Copy link
Contributor

Speaking of which what has all this discussion have to do with the PR itself?

Maybe to check whether those discrepancies can witness possible issues in the code that are not easily realized by just glacing at the code itself?

@mmusich
Copy link
Contributor Author

mmusich commented May 20, 2024

Maybe to check whether those discrepancies can witness possible issues in the code that are not easily realized by just glacing at the code itself?

I can't see how is that even possible. Can you please elaborate?

@perrotta
Copy link
Contributor

I can't see how is that even possible. Can you please elaborate?

Wrong code can give wrong results (can you understand it?).
That's why I'm asking you if the results look correct to you (supposedly the expert).
If you confirm that you don't see issues in the results, I don't have other suggestions that the code is potentially wrong and I can sign this paper

@mmusich
Copy link
Contributor Author

mmusich commented May 20, 2024

@perrotta

Wrong code can give wrong results (can you understand it?).

yes I can understand it, but this seems out of place with your previous line of inquiry.
Based on #44994 (comment) you seemed to imply that the code proposed here could produce different phases of the Z → µµ mass bias vs muon φ depending on the input aligned geometry.
I can't see how that is possible: as here I am just

  • adding more control histograms to produce 2D maps (not previously available).
  • adding a new plotting macro in order to better distinguish the differences between different alignments.

thus, the underlying physics is not changed by any of the updates in this PR.
Just to demonstrate the point above I also produced the plots one can currently get with the Zmumumerge.cc class and collected them here. As you can see the different phases are already visible (albeit less clearly) already here and here.

That's why I'm asking you if the results look correct to you (supposedly the expert).

the result looks compatible with the expectations from the previous code and in line with the conceivable changes induced by a re-alignment campaign.

If you confirm that you don't see issues in the results, I don't have other suggestions that the code is potentially wrong and I can sign this paper

I confirm I don't see issues with the results. If you are still unsure about the soundness of the update I can advice you to get hold of some domain expert within the alca/db group.

@TomasKello
Copy link
Contributor

TomasKello commented May 20, 2024

Hi @perrotta I can confirm there is nth to be worried of the actual content of the plots. We do have a working version of the dimuon mass validation tool which produces identical plots in terms of content. Marco's version is adding some new features, plot types and fixes style of the existing plots, etc. Indeed mp3747 is supposed to mitigate strong bias observed for Alignment in prompt.

@perrotta
Copy link
Contributor

+1

  • Of course this PR can be signed. I only find a bit funny that someone complains when we ask to confirm the correctness of some curious result that sorts out of the comparisons used to test it :-)

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

@francescobrivio
Copy link
Contributor

@cms-sw/orp-l2 a kind ping to merge this fully-sign PR since its backport (#45002) will eventually be needed for data-taking (not urgently).

Cheers,
Francesco as ORM

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit c52102f into cms-sw:master May 21, 2024
11 checks passed
@mmusich mmusich deleted the mm_dev_improve_DiMuonValidation branch May 21, 2024 15:03
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.

6 participants