Skip to content

Conversation

@jan-kaspar
Copy link
Contributor

PR description:

Up to now, the z sign of diamond-RP geometry has been wrong (i.e. inconsistent with other PPS RPs).

This PR brings a fix:

  • for Run3 geometry, the fix is applied directly in the geometry XML file
  • for Run2 geometry, the fix is applied in the geometry builder code - in order to avoid the need to re-upload the geometry to DB, revalidate, ...

Thanks to the fix, the mitigation code is removed from:

  • proton-reco module
  • direct-simulation module

This is a technical PR, no change in results is expected.

PR validation:

Below few snippets from PPS geometry print out (module CTPPSGeometryInfo). ce stands for the detector centre, the z component is the last number.

  • 2018, before PR:
2054160384 (diamd RP  16) | ce=(+73.050, -0.000, +215700.000)
2070937600 (diamd RP 116) | ce=(+73.050, -0.000, -215700.000)

2054160384 (diamd RP  16, plane 0, channel  0) | ce=(+3.325, +0.000, +215679.449)
2054164480 (diamd RP  16, plane 0, channel  1) | ce=(+4.825, +0.000, +215679.449)

2070937600 (diamd RP 116, plane 0, channel  0) | ce=(+3.325, +0.000, -215720.551)
2070941696 (diamd RP 116, plane 0, channel  1) | ce=(+4.825, +0.000, -215720.551)
  • 2018, after PR:
2054160384 (diamd RP  16) | ce=(+73.050, -0.000, -215700.000)
2070937600 (diamd RP 116) | ce=(+73.050, -0.000, +215700.000)

2054160384 (diamd RP  16, plane 0, channel  0) | ce=(+3.325, +0.000, -215679.449)
2054164480 (diamd RP  16, plane 0, channel  1) | ce=(+4.825, +0.000, -215679.449)

2070937600 (diamd RP 116, plane 0, channel  0) | ce=(+3.325, +0.000, +215720.551)
2070941696 (diamd RP 116, plane 0, channel  1) | ce=(+4.825, +0.000, +215720.551)
  • 2021, after PR:
2054160384 (diamd RP  16) | ce=(+72.050, -0.000, -215700.000)
2056257536 (diamd RP  22) | ce=(+72.050, -0.000, -215078.000)
2070937600 (diamd RP 116) | ce=(+72.050, -0.000, +215700.000)
2073034752 (diamd RP 122) | ce=(+72.050, -0.000, +215078.000)

2054160384 (diamd RP  16, plane 0, channel  0) | ce=(+2.325, +0.000, -215720.551)
2054164480 (diamd RP  16, plane 0, channel  1) | ce=(+3.825, +0.000, -215720.551)

2070937600 (diamd RP 116, plane 0, channel  0) | ce=(+2.325, +0.000, +215679.449)
2070941696 (diamd RP 116, plane 0, channel  1) | ce=(+3.825, +0.000, +215679.449)

Interpretation of the snippets:

  • 2018, comparison of before and after this PR: only the z component changed sign, as expected
  • comparison 2018 and 2021, both after the PR: the z signs are identical and correct (negative for LHC sector 45, positive for LHC sector 56)

Addition comparison plots (before and after the PR):

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32603/20644

  • This PR adds an extra 28KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32603/20645

  • This PR adds an extra 32KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

A new Pull Request was created by @jan-kaspar for master.

It involves the following packages:

Geometry/VeryForwardData
Geometry/VeryForwardGeometryBuilder
RecoPPS/ProtonReconstruction
Validation/CTPPS

@perrotta, @andrius-k, @Dr15Jones, @makortel, @cvuosalo, @civanch, @ianna, @mdhildreth, @ErnestaP, @cmsbuild, @kmaeshima, @jfernan2, @fioriNTU, @slava77, @jpata can you please review it and eventually sign? Thanks.
@forthommel, @fabiocos 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

@slava77
Copy link
Contributor

slava77 commented Jan 5, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1d5e1d/11979/summary.html
COMMIT: ca280c2
CMSSW: CMSSW_11_3_X_2021-01-05-1100/slc7_amd64_gcc900

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: 2716967
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2716944
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@cvuosalo
Copy link
Contributor

cvuosalo commented Jan 6, 2021

+1

@jfernan2
Copy link
Contributor

jfernan2 commented Jan 6, 2021

+1

@slava77
Copy link
Contributor

slava77 commented Jan 7, 2021

+1

for #32603 ca280c2

  • technical, relocating the z-sign flip from use cases to upstream, the geometry.
  • jenkins tests pass without differences

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2021

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

@qliphy
Copy link
Contributor

qliphy commented Jan 7, 2021

+1

@cmsbuild cmsbuild merged commit e52a179 into cms-sw:master Jan 7, 2021
@jan-kaspar jan-kaspar deleted the fix_diamond_z_sign branch August 31, 2021 08:28
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