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

expand tracker-driven coverage for electron seeds; jetCore mitigation (bp of #35892) #35958

Merged

Conversation

slava77
Copy link
Contributor

@slava77 slava77 commented Nov 2, 2021

Changes in this PR were motivated to mitigate the situation with tracks reconstructed multiple times, one of them jet core after introduction of mkFit to the non-jetCore iterations.

This was presented in TRK POG on Nov 1: https://indico.cern.ch/event/1092090/#3-interplay-in-high-pt-electro

see more details in #35892

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2021

A new Pull Request was created by @slava77 (Slava Krutelyov) for CMSSW_12_1_X.

It involves the following packages:

  • RecoParticleFlow/PFProducer (reconstruction)
  • RecoParticleFlow/PFTracking (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @cbernet, @rovere, @lgray, @lecriste, @hatakeyamak, @ebrondol, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor Author

slava77 commented Nov 2, 2021

@cmsbuild please test

@perrotta
Copy link
Contributor

perrotta commented Nov 2, 2021

backport of #35892

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-50d44a/20194/summary.html
COMMIT: 76ceb8e
CMSSW: CMSSW_12_1_X_2021-11-02-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35958/20194/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: 1171 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 2901440
  • DQMHistoTests: Total failures: 179
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2901239
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@slava77
Copy link
Contributor Author

slava77 commented Nov 2, 2021

@swagata87
I made this backport as a follow up to the discussion in the TRK POG.
Will this actually be used for EGM ID training in 12_1_X?
I thought that the training was expected in time for Nov 9 deadline ("12_2" last open).
Will there be enough time to do it if 12_1 with this PR is built this Friday?

@swagata87
Copy link
Contributor

swagata87 commented Nov 3, 2021

@swagata87 I made this backport as a follow up to the discussion in the TRK POG. Will this actually be used for EGM ID training in 12_1_X? I thought that the training was expected in time for Nov 9 deadline ("12_2" last open). Will there be enough time to do it if 12_1 with this PR is built this Friday?

Hi Slava,
let me explain EGM's plan and my understanding.

  1. Currently we are training PF Egamma ID using samples produced with 12_0 release. The training is almost complete and we expect to make a PR by the end of this week (5th Nov), to make it to 12_2_0_pre2. There is no possibility to include this backport PR is our current PF Egamma ID training.

  2. But, another re-training of PF Egamma ID is planned. That is to be done using samples produced with 12_1 in next months. We informed PPD that we will request O(100M) events to be produced with 12_1 for this retraining. The original reason of this retraining was to include eta-extended-electrons(EEE) in the training of PF Egamma ID. Recall that EEEs went into release in 12_1. I thought that it will be good to have your PR included as well.

I am a bit lost in new and old plans of 12_1 and 12_2 of the next campaign of sample production.
It looks like 12_1 will be used only to produce GEN-SIM. And 12_2 will be used to run RECO and PAT step.
If that is the case, then this backport is not necessary as it concerns RECO step.
Apologies for the confusion.
But if the RECO step is to be run using 12_1 then having this backport is a good idea.

@slava77
Copy link
Contributor Author

slava77 commented Nov 3, 2021

Hi Swagata,

Thank you for the clarification on the short term plans

2\. But, another re-training of PF Egamma ID is planned. That is to be done using samples produced with 12_1 in next months. We informed PPD that we will request O(100M) events ...

the description for this sounds like the plan for what's currently called 12_2 release (recall that the indexing/naming of the releases changed about a week ago).

@rappoccio please check/confirm about large physics reco samples in 12_2 (vs not in 12_1).

@swagata87
Copy link
Contributor

tagging Linda
@lfinco

@slava77
Copy link
Contributor Author

slava77 commented Nov 4, 2021

+reconstruction

for #35958 76ceb8e

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 4, 2021

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_1_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_2_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented Nov 4, 2021

Thank you @slava77
This "nice to have" PR could delay 12_1_0, which is otherwise ready to be cut even now, by about 1 day. Such a short delay doesn't seem a big issue to me: but please @rappoccio let us know which is the preferred plan for PPD

@rappoccio
Copy link
Contributor

Just to reiterate what we discussed during the meeting: If it's really only a day or two, we can wait to include it in 12.1.0.

@perrotta
Copy link
Contributor

perrotta commented Nov 4, 2021

The master version of this PR, #35892, was just merged. Unless unexpected issues will show up from the nightly builds, tomorrow we can merge this PR in 12_1_X and start building 12_1_0

From the discussions in the github thread of #35892 it must be clear that this PR will bring

  • small differences in egamma related objects
  • about 1% increase of fake electrons in the QCD sample

and the full extent of what above can only be assessed with a complete validation.

According to the declared purposed of 12_1_0 I see no issue in merging it, even if the exact impact on egamma objects cannot be quantified yet. I'm just pointing it out here in case it was not evident to anybody.

@perrotta
Copy link
Contributor

perrotta commented Nov 4, 2021

+1

  • Let merge this PR so that it can be tested with the nightly 12_1_X IB
  • The master version wasn't fully tested in the IBs yet, but I don't expect problems with it: at worst, we will revert this tomorrow morning and build 12_1_0 as well.

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

6 participants