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

[HGCal] dEdx weights and thickness correction factors for V10 (D41) #26354

Merged
merged 3 commits into from Apr 12, 2019

Conversation

apsallid
Copy link
Contributor

@apsallid apsallid commented Apr 4, 2019

PR description:

This PR is (re)based on top of #26301 .

It includes the work done with @rovere, @felicepantaleo, @amartelli, @clelange on the dEdX weights and thickness correction factors for the V10 (D41) geometry.

PR validation:

The slides describing the calculation were presented in the HGCal DPG and can be seen in this link.

if this PR is a backport please specify the original PR:

It is based on a new set of Era's and it is not a back porting.

@felicepantaleo @rovere @amartelli @clelange @cseez @bsunanda @kpedro88 @malgeri

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26354/9072

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

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

It involves the following packages:

Configuration/Eras
Configuration/Geometry
Configuration/PyReleaseValidation
Configuration/StandardSequences
RecoLocalCalo/HGCalRecProducers

@perrotta, @pgunnell, @prebello, @Dr15Jones, @cvuosalo, @civanch, @slava77, @ianna, @kpedro88, @cmsbuild, @franzoni, @mdhildreth, @zhenhu, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @vandreev11, @edjtscott, @sethzenz, @vargasa, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @lgray, @cseez, @pfs, @VinInn, @dgulhan, @kpedro88 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

@perrotta
Copy link
Contributor

perrotta commented Apr 4, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33957/console Started: 2019/04/04 14:05

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

Comparison job queued.

phase2_hgcalV9.toModify( dEdX, weights = dEdX_weights_v9 )

dEdX_weights_v10 = cms.vdouble(0.0, # there is no layer zero
8.894541, # Mev
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't there a place in the DB for things like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

there is no HGCal DB yet. This has been an outstanding issue for years, and probably will continue to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • does it mean there is no plan to keep HGCAL conditions data in DB?
  • or does it mean that getting data to the DB is so complicated that it's easier to embed all variants of it into the CMSSW release?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure there is a plan to keep HGCal conditions in the DB on the scale of the installation timeline. But in general, at least in my experience, setting up and maintaining DB objects is quite person- and time-intensive. I am not sure there is any existing effort in the DPG devoted to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@slava77
it is very clear to the DPG that the most appropriate (only?) way to take care of these calibration constants is via the EventSetup+DB. On the other hand the current situation is still quite fluid and we prefer to have it fixed and available quickly rathter than going through the cumbersome procedure of switching to the DB. Needless to say this switch will come, but I do not have, as of today, a timescale.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26354/33957/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: 3139747
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3139549
  • 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

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2019

Why is github reporting about conflicts which are apparently in some PPS packages, not touched by this PR?

@kpedro88
Copy link
Contributor

kpedro88 commented Apr 5, 2019

I am not sure. but #26301 is merged now, so this PR should be rebased on the latest IB.

@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-26354/9226

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

Pull request #26354 was updated. @perrotta, @cmsbuild, @kpedro88, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Apr 11, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 11, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34145/console Started: 2019/04/11 11:22

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3140495
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3140297
  • 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

@kpedro88
Copy link
Contributor

+upgrade
@slava77 this one has been identified as priority for L1 TDR

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2019

+1

for #26354 7e0cac3

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no differences (HGCAL v10 is not in the tested workflows).
  • the signoff is mainly based on similarities of values wrt v9 and the general code quality

@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

+1

@cmsbuild cmsbuild merged commit de2816d into cms-sw:master Apr 12, 2019
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

8 participants