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 fixedGridRhoFastjetAllTmp to MiniAOD event content #26420

Merged
merged 1 commit into from Apr 14, 2019

Conversation

Sam-Harper
Copy link
Contributor

PR description:

The E/gamma energy regressions use fixedGridRhoFastJetAllTmp as an input. While this is very similar to fixedGridRhoFastJetAll (its done on PF candidates before e/gamma feeds back its energy corrected electrons) it would be nice to have it in the miniAOD as we can use it to exactly reproduce the original application of the regression. Its already in AOD, this corrects the oversight that it is not in MINIAOD

Its a single double added to the event and there are already several fixedGridRhoFastjet flavours so it shouldnt have any impact,.

@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-26420/9192

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/PatAlgos

@cmsbuild, @perrotta, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @rappoccio, @HeinerTholen, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @acaudron, @jdolen, @drkovalskyi, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder 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 Apr 10, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 10, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34120/console Started: 2019/04/10 14:07

@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-26420/34120/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3140495
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3140297
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@slava77
Copy link
Contributor

slava77 commented Apr 11, 2019

@Sam-Harper
please elaborate why fixedGridRhoFastJetAll can not be used for your purposes.
I'm trying to understand if we are mitigating a logic error or if the Tmp has to be used from basic principles.

@Sam-Harper
Copy link
Contributor Author

Because fixedGridRhoFastJetAll uses the electron/photon regression as input. Therefore using it as an input to the electron/photon regression creates a circular dependence.

Now we can keep the status quo and during reco just fixedGridRhoFastJetAllTmp for the initial regression application and then fixedGridRhoFastJetAll for any recalculation of the regression. Admittedly the difference is very small. But I figured for the sake of one double in the event, it would be nice to be in the miniAOD and then we can exactly reproduce the regression used in reco.

Its not a super strong request, it just would make my life a bit easier when debugging and the like.

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2019

assign xpog

to confirm the output content changes in miniAOD

@cmsbuild
Copy link
Contributor

New categories assigned: xpog

@fgolf,@peruzzim you have been requested to review this Pull request/Issue and eventually sign? Thanks

@peruzzim
Copy link
Contributor

+xpog

it would be better to use a more representative name than "Tmp", but I guess it's something cumbersome to change elsewhere (also maintaining compatibility for other releases)

@Sam-Harper
Copy link
Contributor Author

it would be better to use a more representative name than "Tmp", but I guess it's something cumbersome to change elsewhere (also maintaining compatibility for other releases)

Agreed but that is the name of the quantity set in 2014. It makes some sense if you know the provenance. Because there is a bunch of circular dependences in particle flow, you end up having gedPhotonsTmp, gedGsfElectronsTmp, particleFlowTmp so from that convention, it makes sense. It is already available in the AOD.

Btw adding this variable will not overlly confuse the rho situation, there are already many flavours.
double "fixedGridRhoAll" "" "RECO"
double "fixedGridRhoFastjetAll" "" "RECO"
double "fixedGridRhoFastjetAllCalo" "" "RECO"
double "fixedGridRhoFastjetCentral" "" "RECO"
double "fixedGridRhoFastjetCentralCalo" "" "RECO"
double "fixedGridRhoFastjetCentralChargedPileUp" "" "RECO"
double "fixedGridRhoFastjetCentralNeutral" "" "RECO"

@peruzzim
Copy link
Contributor

ok, thanks for the explanation

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2019

+1

for #26420 7f41cc1

  • code changes are in line with the PR description
  • jenkins tests pass and comparisons with the baseline show differences only in the event content of the mininAOD outputs, as expected.
    The use case seems somewhat reasonable.

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 4591ed2 into cms-sw:master Apr 14, 2019
@santocch
Copy link

santocch commented May 2, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2019

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

6 participants