Skip to content

Conversation

@sgrzegorz
Copy link
Contributor

PR description:

The idea behind this pull request is to improve the association cuts mechanism by providing better and more flexible
way of handling association cuts - the rules to match the local tracks from the near RP to those from the far RP.

Code changes for association cuts include

  • cuts (mean and threshold) can be functions of proton kinematics (x_near and y_near) and xangle.
  • cuts can be stored as a standard EventSetup object. Can be saved in the database. Can be associated with arbitrary IOV

The update is fully backward compatible - we expect no changes for Run2 workflows. The new flexibility is only exploited in Run3 configurations.

Once this PR is merged, we plan to upload the cuts into the DB. Eventually, we plan to issue a small follow PR where the cuts are consumed from the DB in the standard reco workflows.

PR documentation includes:

1_sroka_pps_association_cuts.pdf

2_kaspar_pps_association_cuts.pdf

PR validation:

The plots below compare results before (blue solid) and after this PR (red dashed):

  1. for Run2 data reco: reco_cmp.pdf
  • no difference as expected
  1. for direct simulation: dirsim_cmp.pdf
  • no difference for Run2 workflows
  • the difference in 2021 row is due to #35177 (which is not included in CMSSW_12_1_0_pre2 used as the baseline)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35248/25223

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • CalibPPS/ESProducers (alca)
  • CondCore/CTPPSPlugins (db)
  • CondCore/Utilities (db)
  • CondFormats/DataRecord (db, alca)
  • CondFormats/PPSObjects (alca)
  • CondTools/CTPPS (db)
  • RecoPPS/ProtonReconstruction (reconstruction)
  • Validation/CTPPS (dqm)

@malbouis, @andrius-k, @yuanchao, @kmaeshima, @ErnestaP, @ahmad3213, @rvenditti, @cmsbuild, @jpata, @jfernan2, @slava77, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@tocheng, @fabferro, @jan-kaspar, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

@sgrzegorz can you please add subsystem name to PR title (CT-PPS)?

@tvami
Copy link
Contributor

tvami commented Sep 13, 2021

@sgrzegorz and please also clean up the commit history with a git squash, thanks!

@sgrzegorz sgrzegorz changed the title Association cut update CT-PPS: Association cut update Sep 13, 2021
@slava77
Copy link
Contributor

slava77 commented Sep 13, 2021

@cmsbuild please test

@sgrzegorz
Copy link
Contributor Author

sgrzegorz commented Sep 13, 2021

@sgrzegorz and please also clean up the commit history with a git squash, thanks!

@tvami @jan-kaspar I squashed the commits. Here is what I did:

pick aa646ef Added PPSAssociationCutsESSource Class
pick c561cf3 Database connection ---------------------------> Database connection
s ac057ec Do pull request fixes and refactor code ----> Database connection
pick 278aa9d Fixes-------->use TF1 class and store association cuts as strings for more flexibi…
s b5f7e45 Fixes------------->use TF1 class and store association cuts as strings for more flexibi…
pick fd2b55e add more plots
pick 7b6d904 update association-cut model for 2021
pick d09e9f4 code clean up
pick 1a5195e code format applied
pick 0d05534 added 2022 association cuts data ------------------------> add 2022 config and remove 2021 config from the default list
s e7b67fc remove 2021 config from the default list ----------------->add 2022 config and remove 2021 config from the default list

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35248/25237

@cmsbuild
Copy link
Contributor

Pull request #35248 was updated. @malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @rvenditti, @cmsbuild, @jpata, @jfernan2, @slava77, @ggovi, @francescobrivio, @pbo0, @tvami can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Sep 13, 2021

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

Pull request #35248 was updated. @malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @rvenditti, @cmsbuild, @jpata, @jfernan2, @slava77, @ggovi, @francescobrivio, @pbo0, @tvami can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Sep 20, 2021

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d6f42/18759/summary.html
COMMIT: db0a07d
CMSSW: CMSSW_12_1_X_2021-09-19-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35248/18759/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d6f42/18759/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d6f42/18759/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 40
  • DQMHistoTests: Total histograms compared: 3211080
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3211052
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
  • Checked 169 log files, 37 edm output root files, 40 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Sep 20, 2021

+alca

@tvami
Copy link
Contributor

tvami commented Sep 20, 2021

+db

@tvami
Copy link
Contributor

tvami commented Sep 20, 2021

@jfernan2 @jpata this PR was rebased since you last signed, please re-sign when you get a chance

@jfernan2
Copy link
Contributor

+1

@jpata
Copy link
Contributor

jpata commented Sep 21, 2021

+reconstruction

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

@qliphy
Copy link
Contributor

qliphy commented Sep 22, 2021

+1

@cmsbuild cmsbuild merged commit 4a8f106 into cms-sw:master Sep 22, 2021
@jan-kaspar jan-kaspar deleted the association_cut_update branch September 27, 2021 09:32
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.

10 participants