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

Add HiGenEvent object to heavy-ion miniAOD event content #36961

Merged
merged 2 commits into from
Mar 4, 2022

Conversation

mandrenguyen
Copy link
Contributor

PR description:

The HIGenEvent object stores a few gen-level variables that are relevant for heavy ion collisions. It's stored in AOD, we would like to keep it for miniAOD, too. The size of the object is negligible

PR validation:

Tested w/ wf 159

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36961/28316

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mandrenguyen (Matthew Nguyen) for master.

It involves the following packages:

  • PhysicsTools/PatAlgos (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@AlexDeMoor, @rappoccio, @gouskos, @jdolen, @JyothsnaKomaragiri, @ahinzmann, @schoef, @emilbols, @jdamgov, @mbluj, @nhanvtran, @gkasieczka, @hatakeyamak, @gpetruc, @azotz, @mariadalfonso, @demuller, @andrzejnovak, @seemasharmafnal, @mmarionncern 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

@jpata
Copy link
Contributor

jpata commented Feb 15, 2022

@cmsbuild please test

@jpata
Copy link
Contributor

jpata commented Feb 15, 2022

For posterity, can you post some more detailed information about the size?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b9a622/22432/summary.html
COMMIT: d7a114d
CMSSW: CMSSW_12_3_X_2022-02-14-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36961/22432/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 46
  • DQMHistoTests: Total histograms compared: 3764435
  • DQMHistoTests: Total failures: 13
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3764399
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 45 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 193 log files, 42 edm output root files, 46 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Feb 17, 2022

assign xpog

@cmsbuild
Copy link
Contributor

New categories assigned: xpog

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

@mandrenguyen
Copy link
Contributor Author

mandrenguyen commented Feb 17, 2022

@jpata It's just a few floats, I think. 3 kb per event if I understand the output of edmDumpEventSize correctly. It's near the bottom in the list of collections. Should be negligible.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36961/28386

@cmsbuild
Copy link
Contributor

Pull request #36961 was updated. @gouskos, @clacaputo, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso can you please check and sign again.

@clacaputo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b9a622/22512/summary.html
COMMIT: 0809418
CMSSW: CMSSW_12_3_X_2022-02-18-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36961/22512/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 14 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3965143
  • DQMHistoTests: Total failures: 25
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3965095
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

clacaputo commented Feb 23, 2022

@jpata It's just a few floats, I think. 3 kb per event if I understand the output of edmDumpEventSize correctly. It's near the bottom in the list of collections. Should be negligible.

Hi @mandrenguyen , on how many events have you performed this measurement?

Do you have a reference size of an event for HI?

@clacaputo
Copy link
Contributor

Hello @mandrenguyen , any update on this #36961 (comment)?
Thanks

@mandrenguyen
Copy link
Contributor Author

@clacaputo The added size is negligible. I suggest just running wf 159 to check it.

@mandrenguyen
Copy link
Contributor Author

The ratio for 1 event in 159 is 2104127./2103802. = 1.0001545
wf 159 runs peripheral events so lighter than the average PbPb event. In our normal embedded MC this extra collection will be even more negligible.

@clacaputo
Copy link
Contributor

+reconstruction

@mariadalfonso
Copy link
Contributor

+xpog

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2022

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Mar 4, 2022

+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.

7 participants