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

Adding HGCAL SuperCluster Regressions #32901

Merged
merged 6 commits into from Feb 24, 2021

Conversation

Sam-Harper
Copy link
Contributor

PR description:

First this PR is a collaboration with @SohamBhattacharya and @swagata87

This PR extends the supercluster energy correction to support the HGCAL. The supercluster correction class was modernised during this process. Additionally the new HGCalShowerHelper class had to be restructured to give it better const behaviour (ie its methods are now all const).

The HGCalShowerHelper class had an internal cache. This cache has now been separated out to its own object, which now corresponds to a specific object and selection cuts. Finally it was deemed that the rVal makes no sense if divided by another energy than the raw energy so the accessor was changed to force this and avoid accidents.

The primary purpose was to enable this energy correction for the phase II HLT. It has been tested on 100K dielectron events and the performance is as expected for both reco and HLT (modulo a minor bug in reco which has now been fixed)

No changes are expected to non-phase-II workflows beyond the HLT superclusters now getting a resolution estimate (which they dont use) and the only change in phase-II workflows are HGCAL superclusters are now corrected.

PR validation:

super cluster energy of 500 RelValZEE Run3 events
image
image
image
image

There was a minor bug in the offline HGCAL regression so that is being redone but the HLT HGCAL regression is below
image

if this PR is a backport please specify the original PR and why you need to backport that PR:

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32901/21129

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

Configuration/AlCa
DataFormats/WrappedStdDictionaries
RecoEcal/EgammaClusterAlgos
RecoEcal/EgammaClusterProducers
RecoEgamma/EgammaHLTProducers
RecoEgamma/EgammaTools

@perrotta, @smuzaffar, @Dr15Jones, @malbouis, @makortel, @slava77, @christopheralanwest, @Martin-Grunewald, @cmsbuild, @yuanchao, @tlampen, @jpata, @fwyzard, @pohsun, @francescobrivio can you please review it and eventually sign? Thanks.
@fabiocos, @lecriste, @makortel, @rchatter, @jainshilpi, @sobhatta, @Fedespring, @lgray, @Martin-Grunewald, @calderona, @HuguesBrun, @wddgit, @thomreis, @afiqaize, @tocheng, @mmusich, @argiro, @rovere, @varuns23, @cericeci, @ram1123 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 Feb 12, 2021

enable profiling

@slava77
Copy link
Contributor

slava77 commented Feb 12, 2021

@cmsbuild please test

@slava77
Copy link
Contributor

slava77 commented Feb 13, 2021

@smuzaffar @mrodozov

In the jenkins pages I found "PR #32901 slc7_amd64_gcc900 prefiling "
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-wait-deployment/231/
which looks like a buggy setup.
please check if the profiling job was actually started properly.
Thank you.

@smuzaffar
Copy link
Contributor

@slava77 , yes there was a typo and this is fixed now. profiling job is running now

@slava77
Copy link
Contributor

slava77 commented Feb 13, 2021

@slava77 , yes there was a typo and this is fixed now. profiling job is running now

@smuzaffar
Thank you.

do I understand correctly that this cms-cmpwg/profiling@249813e#diff-2d467ee7db6fbd691d551a712660bc304f2ecad307660feed209a5f400cf1a0eR40
is the reason why the profiling jobs are running much slower now (since about a week)? The number of events was increased from 20 to 100. I think that this is an overkill for per-PR/per-IB tests.

@smuzaffar
Copy link
Contributor

you are right @slava77 that is one of the reason. @gartung , looks like for pr profiling job is using the defaults from the cmpwg repo instead of getting it from Jenkins job. What are the env profiling scripts needs from Jenkins job?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5906b2/12867/summary.html
COMMIT: affd483
CMSSW: CMSSW_11_3_X_2021-02-12-1100/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32901/12867/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-5906b2/11634.911_TTbar_14TeV+2021_DD4hep+TTbar_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST+ALCA

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1050 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2750846
  • DQMHistoTests: Total failures: 1565
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2749258
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@gartung
Copy link
Member

gartung commented Feb 14, 2021

Sorry about that. I was doing some profiling work at NERSC and changed the default to 100. The environment variable EVENTS should override the default in the script.

@gartung
Copy link
Member

gartung commented Feb 14, 2021

@smuzaffar I updated the pr profling job to set EVENTS as the ib profiling jobs does.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32901/21206

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

Pull request #32901 was updated. @perrotta, @smuzaffar, @Dr15Jones, @malbouis, @makortel, @slava77, @christopheralanwest, @Martin-Grunewald, @cmsbuild, @yuanchao, @tlampen, @jpata, @fwyzard, @pohsun, @francescobrivio can you please check and sign again.

@Martin-Grunewald
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5906b2/13037/summary.html
COMMIT: c8f2356
CMSSW: CMSSW_11_3_X_2021-02-22-2300/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32901/13037/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1046 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2750983
  • DQMHistoTests: Total failures: 1560
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2749401
  • 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

@tlampen
Copy link
Contributor

tlampen commented Feb 23, 2021

+alca

@Martin-Grunewald
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Feb 23, 2021

@makortel
Copy link
Contributor

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

@qliphy
Copy link
Contributor

qliphy commented Feb 24, 2021

+1

@cmsbuild cmsbuild merged commit c861bc0 into cms-sw:master Feb 24, 2021
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