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

[PPS] Conditions format update for diamond timing calibration #26207

Merged
merged 10 commits into from Mar 26, 2019

Conversation

forthommel
Copy link
Contributor

@forthommel forthommel commented Mar 18, 2019

PR description:

A new readout mode for timing calibration JSONs is introduced for the PPSTimingCalibration ESProducer. This allows the rechit-level calibration of diamond detectors to be submitted in a follow-up PR (HPTDC time over threshold calibration conditions produced outside this scope).

PR validation:

To be tested along with cms-sw/cmsdist#4785, which enables to parse and produce the payload from an external (example) file through the CondTools/CTPPS/test/ppsTimingCalibrationWriter_cfg.py utility.
Local reco branch maintained in parallel, to be submitted when this PR reaches a more mature state. Validation SW tools part of this follow-up PR.

if this PR is a backport please specify the original PR: N/A

Before submitting your pull requests, make sure you followed this checklist:

cc: @fabferro @jan-kaspar @jjhollar

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26207/8810

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @forthommel (Laurent Forthomme) for master.

It involves the following packages:

CondFormats/CTPPSReadoutObjects
CondTools/CTPPS

@ggovi, @cmsbuild, @pohsun, @franzoni, @tocheng can you please review it and eventually sign? Thanks.
@mmusich, @jan-kaspar, @tocheng, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@forthommel forthommel changed the title Pps diam timing cond format [PPS] Conditions format update for diamond timing calibration Mar 18, 2019
@forthommel
Copy link
Contributor Author

Could someone please trigger the tests? Thanks in advance!

@tocheng
Copy link
Contributor

tocheng commented Mar 18, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 18, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33642/console Started: 2019/03/18 16:35

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26207/33642/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3114829
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3114631
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@forthommel
Copy link
Contributor Author

Is there any way we can help proceeding with this PR review? This one is a prerequisite for our forthcoming offline reco patch introducing timing calibration in rechits, much needed prior to UL rereco.

@tocheng
Copy link
Contributor

tocheng commented Mar 21, 2019

@forthommel We will review it soon.

@pohsun
Copy link

pohsun commented Mar 24, 2019

@forthommel Do you have some consideration for not reverting back the argument names of PPSTimingCalibration::parameters, PPSTimingCalibration::timeOffset, and PPSTimingCalibration::timePrecision? It would be better to use meaningful names.

@forthommel
Copy link
Contributor Author

``

@forthommel Do you have some consideration for not reverting back the argument names of PPSTimingCalibration::parameters, PPSTimingCalibration::timeOffset, and PPSTimingCalibration::timePrecision? It would be better to use meaningful names.

@pohsun Unfortunately we discovered the overall object footprint would be amended by a change of indexing variables names. As the two subdetectors’ offline codes are sharing these calibrations objects and subsequent indexing variables (but with a slightly different meaning of the indexing parameters) we hence only modified the getters instead. What would you suggest then?

  • modify the indexing variables names, hence modify the footprint, and upload a new version of the calibrations for the Totem timing detectors, or
  • leave the indexing variables names untouched while modifying the API for the calibration object (thus not break the backward-compatibility)?

@pohsun
Copy link

pohsun commented Mar 25, 2019

+1

@forthommel Thanks for the explanation. Given the meaning in the two subdetectors is different. I thought current treatment is fine enough.

@forthommel
Copy link
Contributor Author

Many thanks, @pohsun! @ggovi?

@fabiocos
Copy link
Contributor

@ggovi could you please review?

@ggovi
Copy link
Contributor

ggovi commented Mar 26, 2019

+1

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

@forthommel
Copy link
Contributor Author

Many thanks, @ggovi!

# load calibrations from JSON file
process.load('CondFormats.CTPPSReadoutObjects.ppsTimingCalibrationESSource_cfi')
process.ppsTimingCalibrationESSource.calibrationFile = cms.FileInPath('RecoCTPPS/TotemRPLocal/data/timing_offsets_ufsd_2018.dec18.cal.json')
process.ppsTimingCalibrationESSource.calibrationFile = cms.FileInPath('RecoCTPPS/TotemRPLocal/data/timing_calibration_diamond_2018_mar19.ex.json')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forthommel related to your comment cms-data/RecoCTPPS-TotemRPLocal#3 (comment) : I understand this is just a test, but if that file is not merged the test will be broken and in practice useless. Will json files be routinely used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are perfectly right. As it is now, this test is only used as a method to produce the sqlite payload prior to filling the database, therefore only indirectly in production. Same comment for the ESProducer itself. So no, theses JSON files will not be routinely used in production.

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 2bbec20 into cms-sw:master Mar 26, 2019
@forthommel forthommel deleted the ppsDiamTiming_condFormat branch March 26, 2019 23:52
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.

None yet

6 participants