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

Adapting VID & Nano to the new E/gamma UL format + adding HEEP V7.1 #26486

Merged

Conversation

Sam-Harper
Copy link
Contributor

PR description:

This is the adaption PR for #26485 which was originally #26389 (this PR is currently exactly the same as that PR just ordered). It contains all of #26485, I was not sure how to proceed here as it needs those changes. This PR starts from commit 2a8a8bf.

Testing on going once the first PR is tested.

Relative to #2648, there should be no change in output with the exception of the HEEP IDs which will suffer from rounding effects leading to a difference to trk isolation when now calculated using tracks rather than packed candidates.

@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-26486/9341

  • This PR adds an extra 256KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DataFormats/EgammaCandidates
PhysicsTools/NanoAOD
PhysicsTools/PatAlgos
RecoEcal/EgammaCoreTools
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaPhotonProducers
RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification
RecoHI/Configuration
RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @fgolf, @slava77, @santocch, @peruzzim can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jainshilpi, @hatakeyamak, @rappoccio, @argiro, @HeinerTholen, @lgray, @varuns23, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @acaudron, @peruzzim, @jdolen, @drkovalskyi, @ferencek, @mandrenguyen, @yetkinyilmaz, @cbernet, @rovere, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @dgulhan, @clelange, @echapon, @MiheeJo, @jazzitup, @JyothsnaKomaragiri, @mverzett, @yenjie, @kurtejung, @gpetruc, @mariadalfonso, @pvmulder, @bachtis 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

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@Sam-Harper
Copy link
Contributor Author

fixes the issue with the summer 16 HLT preselection. Also at the same time I released I broke the HEEP V5.0 however rather than fix it, I removed it from the release as it is old. Including the no-longer run tests for it.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26486/9362

  • This PR adds an extra 276KB to repository

@cmsbuild
Copy link
Contributor

Pull request #26486 was updated. @perrotta, @slava77, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @fgolf, @tlampen, @pohsun, @santocch, @peruzzim can you please check and sign again.

@peruzzim
Copy link
Contributor

please test

I test it already to see if it runs and proceed with more detailed checks on nanoAOD content

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 23, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34308/console Started: 2019/04/23 22:56

@cmsbuild
Copy link
Contributor

-1

Tested at: 0075a5b

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26486/34308/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS
---> test runtestRecoEgammaPhotonIdentification had ERRORS

@Sam-Harper
Copy link
Contributor Author

Meh, okay I could do that but rabbit hole as I need to adjust the parsers to deal with pat::Photons and have fun with dynamic casting. It could be done but it'll be nasty. I dont think we care enough to do that?

@slava77
Copy link
Contributor

slava77 commented May 14, 2019

Meh, okay I could do that but rabbit hole as I need to adjust the parsers to deal with pat::Photons and have fun with dynamic casting. It could be done but it'll be nasty. I dont think we care enough to do that?

OK, I'm going to leave this to @peruzzim and @fgolf to decide.

I'm fine from the reco side because it looks like the issue is understood and is not affecting what was done in the reco/miniAOD implementation.

@peruzzim
Copy link
Contributor

I would like to discuss with Sam in person tomorrow, I think it will be more effective - let's put this discussion on hold until then, please

@slava77
Copy link
Contributor

slava77 commented May 14, 2019

I would like to discuss with Sam in person tomorrow, I think it will be more effective - let's put this discussion on hold until then, please

sure, your signature will be needed for this PR anyways.

@smuzaffar smuzaffar modified the milestones: CMSSW_10_6_X, CMSSW_11_0_X May 14, 2019
@slava77
Copy link
Contributor

slava77 commented May 14, 2019

+1

for #26486 cffbf72

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show differences in expected places:
    • new IDs are added in miniAOD photons and electrons
  • tests with more events (40K) on nanoAOD workflows running on existing miniAODs from 80X/94X/102X show differences only in rare cases in 80X related to discrepancies in the way how photonIso is filled in the mvaID . This feature is understood and perhaps needs a fix in nanoAOD, but is perhaps small enough (O(1e-3)) that it's OK.

@peruzzim
Copy link
Contributor

+xpog

I tested this PR in the nanoAOD integration area, detailed outcome here cms-nanoAOD#363 (comment) confirming what expected

@slava77
Copy link
Contributor

slava77 commented May 15, 2019

in the meantime, this now became a 11_0_X PR.
We need a backport to 10_6_X.
I'm guessing that the same topic branch can be used and the new PR can be made almost trivially.

@fabiocos
Copy link
Contributor

@Sam-Harper @slava77 indeed this is one of the PRs that we need to port back to 10_6_X

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@santocch
Copy link

+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 be automatically merged.

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

9 participants