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 the regression config for light by light #28051

Merged
merged 1 commit into from Oct 15, 2019

Conversation

Sam-Harper
Copy link
Contributor

Adds the regression config and enables it for the correct modifier for the light by light regression.

No changes expected to any workflow that does not use the light by light modifier.

Note no changes are needed for the supercluster regression as 106X+ uses by default the settings the low pt regression needs for that.

@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-28051/11983

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoEgamma/EgammaTools

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@jainshilpi, @varuns23, @lgray 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

@slava77
Copy link
Contributor

slava77 commented Sep 23, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 23, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2623/console Started: 2019/09/23 15:06

@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-54cf31/2623/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2958092
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2957750
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 145 log files, 15 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2019

@Sam-Harper @rchudasa @christopheralanwest
is there a GT with the new low-pt payloads?
Previously I tested with auto:run2_data and I see that in the master autoCond now this points to 110X_dataRun2_v7, which does not have these payloads.

IIUC, the goal is to validate all the changes also in 11_X. So, this PR is incomplete without an existing GT.

@Sam-Harper
Copy link
Contributor Author

It will exist either today or monday (we were in discussions with AlCa for this)

@Sam-Harper
Copy link
Contributor Author

okay it'll be monday I just ran out of time today

@slava77
Copy link
Contributor

slava77 commented Oct 1, 2019

okay it'll be monday I just ran out of time today

@Sam-Harper
please update on the status.
Thank you.

@Sam-Harper
Copy link
Contributor Author

Tag was created Monday and I'm awaiting on Ruchi to test it.

103X_upgrade2018_realistic_HI_Candidate_2019_09_30_11_25_19
103X_dataRun2_Prompt_Candidate_2019_09_30_11_27_53
110X_upgrade2018_realistic_HI_Candidate_2019_09_30_11_29_28
110X_dataRun2_PromptLike_HI_Candidate_2019_09_30_11_30_43

Once Ruchi confirms the tag is working, I'll ask Alca to make a properly named one

@rchudasa
Copy link
Contributor

rchudasa commented Oct 2, 2019

@slava77 , @Sam-Harper
The global tags are validated.

@slava77
Copy link
Contributor

slava77 commented Oct 3, 2019

@slava77 , @Sam-Harper
The global tags are validated.

Nice.
I guess that now this and #27898 are expected to be updated.
Is that the plan?

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2019

@slava77 , @Sam-Harper
The global tags are validated.

Nice.
I guess that now this and #27898 are expected to be updated.
Is that the plan?

@Sam-Harper
please clarify.
Thank you.

@Sam-Harper
Copy link
Contributor Author

what is there to update?

@Sam-Harper
Copy link
Contributor Author

Ah are you wanting a new auto cond entry or something?

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2019

Ah are you wanting a new auto cond entry or something?

yes;
or, alternatively, to be told that a GT should be selected manually for the tests (separate GTs, I suppose for the master and the 10_3_X) version.

@Sam-Harper
Copy link
Contributor Author

The tags are:
103X_upgrade2018_realistic_HI_Candidate_2019_09_30_11_25_19
103X_dataRun2_Prompt_Candidate_2019_09_30_11_27_53
110X_upgrade2018_realistic_HI_Candidate_2019_09_30_11_29_28
110X_dataRun2_PromptLike_HI_Candidate_2019_09_30_11_30_43

with the 103X now renamed to

103X_dataRun2_Prompt_LowPtPhotonReg_v1
103X_upgrade2018_realistic_HI_LowPtPhotonReg_v1

Best,
Sam

@rchudasa
Copy link
Contributor

@slava77
Is there anything remaining from our side for this PR to be merged?

@slava77
Copy link
Contributor

slava77 commented Oct 14, 2019

@rchudasa and @Sam-Harper
I was looking at the GT diffs in the 11X auto-GT vs 110X_dataRun2_PromptLike_HI_Candidate_2019_09_30_11_30_43 mentioned in #28051 (comment)
https://cms-conddb-prod.cern.ch/cmsDbBrowser/diff/Prod/gts/110X_dataRun2_PromptLike_HI_v6/110X_dataRun2_PromptLike_HI_Candidate_2019_09_30_11_30_43

I see that ecalPFClusterCor2017V2* payloads are older in 110X_dataRun2_PromptLike_HI_Candidate_2019_09_30_11_30_43 with *UL2017To2018V1* names compared to *UL2017To2018V3* in the auto-gt 110X_dataRun2_PromptLike_HI_v6
If I'm not mistaken, the V1 is buggy (https://indico.cern.ch/event/842863/contributions/3562026/attachments/1906927/3149650/updates_2018ULPFCorrections.pdf and https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/4222.html) .
Please check.

@slava77
Copy link
Contributor

slava77 commented Oct 15, 2019

+1

for #28051 17f56d5

  • code changes are in line with the PR description and the follow up discussion (on the GT to use for testing, detached from this PR)
  • jenkins tests pass and comparisons with the baseline show no differences in the standard wf tests
  • local tests with a file from HIForward/HIRun2018A-v1/RAW run 326854 (used in the check out of the code enabled with the egamma_lowPt_exclusive modifier shows roughly expected differences using 110X_dataRun2_PromptLike_HI_Candidate_2019_09_30_11_30_43 for both baseline and this PR tests.
    • Energies of egamma objects change as expected from the new regression application.
    • There are no significant changes in the execution time or the output size

Among the most visible is the gedPhoton energy change
all_sign1089-lblVSorig-lbl_HIForward2018Ac_recoPhotons_gedPhotons__RECO_obj_et

@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 6113aac into cms-sw:master Oct 15, 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

5 participants