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

[RecoEgamma/EgammaTools] Updates to EnergyScaleCorrection class for scales and smearings in UL #29526

Merged
merged 7 commits into from May 9, 2020

Conversation

jainshilpi
Copy link
Contributor

This PR takes care of the updated txt file format which stores the scales and smearing corrections.
Updates to following codes have been done:

(1) EnergyScaleCorrection.cc/h - As Sam suggested, the code can now determine the txt file format and reads accordingly
(2) calibratedEgammas_cff.py - now the default txt file to be read is the UL one. For the last re-reco of 2017, run2_miniAOD_94XFall17 is used
(3) Small change to PhotonEnergyCalibrator/ElectronEnergyCalibrator have been done to avoid problems at the boundary.

The related txt files can be found here: https://github.com/jainshilpi/EgammaAnalysis-ElectronTools/tree/UL2017SSV2/ScalesSmearings (24Feb)

runMatrix passed all the tests.

@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-29526/14792

  • This PR adds an extra 24KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@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-29526/14793

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoEgamma/EgammaTools

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@sobhatta, @afiqaize, @Sam-Harper, @varuns23, @lgray this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Apr 20, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 20, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5796/console Started: 2020/04/20 23:33

@cmsbuild
Copy link
Contributor

-1

Tested at: 755d702

CMSSW: CMSSW_11_1_X_2020-04-20-1100
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e31571/5796/summary.html

I found follow errors while testing this PR

Failed tests: ClangBuild

  • Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors/Fireworks only changes/No short matrix requested (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

Comparison job queued.

@jainshilpi
Copy link
Contributor Author

Is there a workflow in the matrix to test this new correction?
@slava77 @perrotta @jainshilpi Do you need new workflows to test?

These new UL corrections should be applied by default if not in those two eras. Just to mention that in EGM, we have already validated these corrections and can be found here (using EgammaPostRecoTools): https://shilpi-afs.web.cern.ch/shilpi-afs/13TeV/2020/SSvalidation/UL2017/30marchV2/

@slava77
Copy link
Contributor

slava77 commented Apr 23, 2020

These new UL corrections should be applied by default if not in those two eras.

there were no changes in comparisons, which suggests to me (also based on git grep of configs in the release) that only the two eras have the calibrated photons/electrons. So, it looks like there is no default running.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2696435
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2696114
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@cmsbuild cmsbuild mentioned this pull request Apr 24, 2020
@slava77 slava77 mentioned this pull request Apr 30, 2020
41 tasks
@silviodonato silviodonato changed the title Updates to EnergyScaleCorrection class for scales and smearings in UL [RecoEgamma/EgammaTools] Updates to EnergyScaleCorrection class for scales and smearings in UL May 5, 2020
@silviodonato
Copy link
Contributor

These new UL corrections should be applied by default if not in those two eras.

there were no changes in comparisons, which suggests to me (also based on git grep of configs in the release) that only the two eras have the calibrated photons/electrons. So, it looks like there is no default running.

@jainshilpi please note Slava's comment

@jainshilpi
Copy link
Contributor Author

These new UL corrections should be applied by default if not in those two eras.

there were no changes in comparisons, which suggests to me (also based on git grep of configs in the release) that only the two eras have the calibrated photons/electrons. So, it looks like there is no default running.

@jainshilpi please note Slava's comment

Hi Silvio, yes this was discussed in the PPD meetings and I discussed iwth Slava separately. I think Slava already is working on it and may have the workflow soon. So once it is done, I will proceed accordingly.

@slava77
Copy link
Contributor

slava77 commented May 8, 2020

The related txt files can be found here: https://github.com/jainshilpi/EgammaAnalysis-ElectronTools/tree/UL2017SSV2/ScalesSmearings (24Feb)

this requires a PR/update in https://github.com/cms-data/EgammaAnalysis-ElectronTools
Please make a PR for cms-data.
Thank you.

@jainshilpi
Copy link
Contributor Author

I had forgotten to do that, sorry. Just did it.

@slava77
Copy link
Contributor

slava77 commented May 9, 2020

+1

for #29526 5f6a5e0

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2020

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

@silviodonato
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

7 participants