cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard
verified publisher icon cms-sw

Remove a dead assignment of accChargeForToA in the HGCDigitizer

Open perrotta opened this issue 3 years ago • 21 comments

PR description:

Spotted by the static analyzer

PR validation:

It builds succesfully No changes at all expected in the outputs

perrotta avatar Oct 05 '22 09:10 perrotta

assign hgcal-dpg

perrotta avatar Oct 05 '22 09:10 perrotta

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39622/32417

  • This PR adds an extra 16KB to repository

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

    • File SimCalorimetry/HGCalSimProducers/plugins/HGCDigitizer.cc modified in PR(s): #39447

cmsbuild avatar Oct 05 '22 09:10 cmsbuild

New categories assigned: hgcal-dpg

@felicepantaleo,@rovere,@pfs,@cseez you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild avatar Oct 05 '22 09:10 cmsbuild

A new Pull Request was created by @perrotta (Andrea Perrotta) for master.

It involves the following packages:

  • SimCalorimetry/HGCalSimProducers (upgrade, simulation)

@cseez, @civanch, @felicepantaleo, @rovere, @cmsbuild, @AdrianoDee, @srimanob, @mdhildreth, @pfs can you please review it and eventually sign? Thanks. @youyingli, @vandreev11, @sethzenz, @bsunanda, @rovere, @lgray, @cseez, @apsallid, @pfs, @hatakeyamak, @trtomei, @beaucero this is something you requested to watch as well. @perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cmsbuild avatar Oct 05 '22 09:10 cmsbuild

please test

perrotta avatar Oct 05 '22 09:10 perrotta

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39622/32421

  • This PR adds an extra 16KB to repository

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

    • File SimCalorimetry/HGCalSimProducers/plugins/HGCDigitizer.cc modified in PR(s): #39447

cmsbuild avatar Oct 05 '22 11:10 cmsbuild

Pull request #39622 was updated. @cseez, @civanch, @felicepantaleo, @rovere, @cmsbuild, @AdrianoDee, @srimanob, @mdhildreth, @pfs can you please check and sign again.

cmsbuild avatar Oct 05 '22 11:10 cmsbuild

please test

perrotta avatar Oct 05 '22 11:10 perrotta

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39622/32424

  • This PR adds an extra 16KB to repository

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

    • File SimCalorimetry/HGCalSimProducers/plugins/HGCDigitizer.cc modified in PR(s): #39447

cmsbuild avatar Oct 05 '22 12:10 cmsbuild

Pull request #39622 was updated. @cseez, @civanch, @felicepantaleo, @rovere, @cmsbuild, @AdrianoDee, @srimanob, @mdhildreth, @pfs can you please check and sign again.

cmsbuild avatar Oct 05 '22 12:10 cmsbuild

please test

perrotta avatar Oct 05 '22 12:10 perrotta

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c3e35e/28021/summary.html COMMIT: 2ba7f839c50873a36d523d892a25a9fd1cbca18b CMSSW: CMSSW_12_6_X_2022-10-05-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39622/28021/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline: 10224.15 step 5 11634.15 step 3 140.002 step 3 140.034 step 3 140.116 step 3 25202.15 step 5 The results for the comparisons for these workflows could be incomplete This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

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

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c3e35e/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 485 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3391103
  • DQMHistoTests: Total failures: 750
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3390331
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 49 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Oct 05 '22 16:10 cmsbuild

Ciao @perrotta thanks for the changes. I see that all the failed comparisons belong to the only workflow using the premixing which is the only part of the code that this PR touches. I'd have expected 0 regression unless there's some other randomization happening that spoils the reproducibility for different reasons. Suggestions?

rovere avatar Oct 07 '22 09:10 rovere

Ciao @perrotta thanks for the changes. I see that all the failed comparisons belong to the only workflow using the premixing which is the only part of the code that this PR touches. I'd have expected 0 regression unless there's some other randomization happening that spoils the reproducibility for different reasons. Suggestions?

Thank you @rovere for having checked Uhm, no. No regression was expected, not even a different randomization. There must be some flaw in the logic of my "fix", that I cannot spot by now: I will check more carefully later on

perrotta avatar Oct 07 '22 09:10 perrotta

Indeed, there was a mistake...

perrotta avatar Oct 07 '22 12:10 perrotta

please test

perrotta avatar Oct 07 '22 12:10 perrotta

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39622/32478

  • This PR adds an extra 16KB to repository

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

    • File SimCalorimetry/HGCalSimProducers/plugins/HGCDigitizer.cc modified in PR(s): #39447

cmsbuild avatar Oct 07 '22 12:10 cmsbuild

Pull request #39622 was updated. @civanch, @felicepantaleo, @rovere, @cseez, @AdrianoDee, @srimanob, @pfs, @mdhildreth can you please check and sign again.

cmsbuild avatar Oct 07 '22 12:10 cmsbuild

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c3e35e/28107/summary.html COMMIT: fc830b36d6b210197ca8c3f65d70025a6b878636 CMSSW: CMSSW_12_6_X_2022-10-07-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39622/28107/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

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

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c3e35e/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3391103
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3391075
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 49 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Oct 07 '22 17:10 cmsbuild

+1

rovere avatar Oct 07 '22 18:10 rovere

+1

civanch avatar Oct 09 '22 09:10 civanch

+Upgrade

srimanob avatar Oct 28 '22 06:10 srimanob

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

cmsbuild avatar Oct 28 '22 06:10 cmsbuild

@cmsbuild please test

Re-test after 3 weeks.

srimanob avatar Oct 28 '22 06:10 srimanob

@rovere @felicepantaleo , while tests are running could you please have a look at my old comment https://github.com/cms-sw/cmssw/pull/39622#discussion_r990763607 ?

perrotta avatar Oct 28 '22 08:10 perrotta

-1

Failed Tests: RelVals-INPUT Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c3e35e/28587/summary.html COMMIT: fc830b36d6b210197ca8c3f65d70025a6b878636 CMSSW: CMSSW_12_6_X_2022-10-27-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39622/28587/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3384029
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3384001
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 201 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Oct 28 '22 11:10 cmsbuild

hold

  • Reminder that there is a pending question here

rappoccio avatar Oct 28 '22 19:10 rappoccio

hold

rappoccio avatar Oct 28 '22 19:10 rappoccio

Pull request has been put on hold by @rappoccio They need to issue an unhold command to remove the hold state or L1 can unhold it for all

cmsbuild avatar Oct 28 '22 19:10 cmsbuild

No need to keep this PR in standby forever only because it points out a bug in the code: an issue for it has been opened in https://github.com/cms-sw/cmssw/issues/40576 for that Let rerun the tests here, and merge this technical PR if they still succed after 5 months

perrotta avatar Jan 20 '23 12:01 perrotta