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

Modifications to HCAL trigger LUTs #22121

Merged
merged 9 commits into from Feb 17, 2018

Conversation

christopheralanwest
Copy link
Contributor

This PR makes several changes to the generation of the LUTs for HCAL trigger primitive generation:

  • eta assignment for subcomponents of trigger tower 28 is now based on the different sub-components of the the tower rather than using the average eta of the tower.
  • The SiPM nonlinearity correction is made more similar to the offline reconstruction by using an estimate of the full pulse charge rather than a 1 time sample charge
  • Estimates for the charge that falls outside the 2-TS sum used in the HBHE trigger primitives is now ET-dependent, replacing the old ET-independent "Rcalib" correction.

There are also two minor technical changes:

  • SiPM nonlinearity corrections are now not applied for HEP17 TPs in 2017, consistent with the 2017 online usage; this has a negligible effect on any physical quantity
  • Changes to the hardcoded conditions used for future MC scenarios where real conditions do not yet exist; with the changes in this PR, there are no longer any "magic numbers" associated with HCAL trigger conditions

No changes are expected for any quantity that does not use HCAL TPs. In particular, the changes in the pulse shape classes simply provide a different way to initialize the pulse shape information from conditions.

At a recent L1 Trigger Primitives meeting [1], the L1 DPG signed off on the HCAL trigger group's plan to modify the LUT generation procedure. More details can be found in the presentation from that meeting.

For consistency, the HcalLutMetadata conditions should be changed simultaneously with the code, or else an undesired 13% change in the HBHE TP scale will result. For this reason, I'm requesting the update in the 2018 MC GTs.

[1] https://indico.cern.ch/event/700828/

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2018

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

It involves the following packages:

CalibCalorimetry/HcalAlgos
CalibCalorimetry/HcalPlugins
CalibCalorimetry/HcalTPGAlgos
CalibCalorimetry/HcalTPGEventSetup
CalibFormats/HcalObjects

@ghellwig, @cmsbuild, @arunhep, @cerminar, @nsmith-, @rekovic, @franzoni, @thomreis, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @mariadalfonso, @mmusich, @tocheng 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

@abdoulline
Copy link

abdoulline commented Feb 5, 2018

Chris, how difficult would be to get correctionPhaseNS from HcalRecoParams
https://github.com/cms-sw/cmssw/pull/22121/files#diff-5a492b88f75c0ab4001c721c7f12e679R40

@thomreis
Copy link
Contributor

thomreis commented Feb 6, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25919/console Started: 2018/02/06 13:55

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2018

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22121/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2465763
  • DQMHistoTests: Total failures: 227
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2465367
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.02000000003 KiB( 22 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files

@christopheralanwest
Copy link
Contributor Author

The pull request was opened with the understanding that a commit with the needed change to the GTs would be provided [1] by @arunhep. This commit is not yet included, and so the comparison tests will show more discrepancies than expected due to the 13% discrepancy mentioned in the initial message opening this PR.

@abdoulline This should be possible, though it's not the completely trivial change that one might expect. I'm looking into how to implement your request.

[1] https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/3478/1.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2018

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22121/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017+HARVESTNANOAODMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2464162
  • DQMHistoTests: Total failures: 164
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2463829
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.06999999983 KiB( 22 files compared)
  • Checked 111 log files, 9 edm output root files, 27 DQM output files

@christopheralanwest
Copy link
Contributor Author

The changes in the comparison test are as expected:

  • No changes are seen in any plot that does not use HCAL trigger primitives
  • Changes are only observed in 2017, 2018 and 2019 scenarios.
  • In 10224.0 (2017 scenario), the changes are not visible; only per-mille differences in a few histograms. The small impact is expected because the only change for 2017 scenarios is the removal of the SiPM nonlinearity correction in HEP17, to match the absence of this correction in 2017 data.
  • In 10824.0 (2018 scenario) and 11624.0 (2019 scenario), changes are primarily single-bin shifts. In the PR tests, the statistics are too low to probe the changes to a small region of the detector (the changes to the handling of TT28) or high pT (the only energy range for which the SiPM nonlinearity correction in HE is important). Small differences in the TP multiplicity at low ET, as shown in the attached plot, are expected; the correction for the charge leakage outside the two time samples considered in the 2-TS is now ET-dependent, and larger at low ET.

hcaldigisv__hcaldigitask_hcaldigitask_tp_et

@abdoulline
Copy link

@arunhep, @rekovic, @thomreis, @lpernie can you please check and sign ?

@abrinke1
Copy link
Contributor

abrinke1 commented Feb 14, 2018

+1 from L1T DPG

@nsmith-
Copy link
Contributor

nsmith- commented Feb 15, 2018

+1

@lpernie
Copy link
Contributor

lpernie commented Feb 15, 2018

+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)

@fabiocos
Copy link
Contributor

@lpernie I see a comment about a corresponding change in the GT needed to avoid a global shift in the TP scale. Is this addressed?

@lpernie
Copy link
Contributor

lpernie commented Feb 16, 2018

Hi, I see that the GTs have been updated by @arunhep with
bd9022a
and they look fine to me. I do not see the comment you are referring too, could you point it to me?

@fabiocos
Copy link
Contributor

@lpernie @christopheralanwest thanks for the clarification

@fabiocos
Copy link
Contributor

+1

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

9 participants