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

Changes to store Daughters of Kshort and Lambda in the miniAOD prunedGenParticle Collection #42464

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Changes to store Daughters of Kshort and Lambda in the miniAOD prunedGenParticle Collection #42464

merged 1 commit into from
Aug 7, 2023

Conversation

gmelachr
Copy link
Contributor

@gmelachr gmelachr commented Aug 3, 2023

Modify prunedGenParticles_cfi.py to store daughters of Kshort and Lambda in the prunedGenParticle collection in order to use the daughter's production vertices for Kshort and Lambda lifetime measurements. The changes have been tested locally.

@cmsbuild cmsbuild added this to the CMSSW_13_3_X milestone Aug 3, 2023
@gmelachr gmelachr changed the title Changes to store Daughters of Kshort and Lambda in the miniAOD pruned… Changes to store Daughters of Kshort and Lambda in the miniAOD prunedGenParticle Collection Aug 3, 2023
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42464/36467

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2023

A new Pull Request was created by @gmelachr (Georgios Melachroinos) for master.

It involves the following packages:

  • PhysicsTools/PatAlgos (xpog, reconstruction)

@cmsbuild, @simonepigazzini, @mandrenguyen, @clacaputo, @vlimant can you please review it and eventually sign? Thanks.
@AlexDeMoor, @rappoccio, @gouskos, @jdolen, @JyothsnaKomaragiri, @ahinzmann, @AnnikaStein, @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, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@simonepigazzini
Copy link
Contributor

enable nano

@simonepigazzini
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-31e805/34066/summary.html
COMMIT: b032b2a
CMSSW: CMSSW_13_3_X_2023-08-03-1100/el8_amd64_gcc11
Additional Tests: NANO
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42464/34066/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 13 lines from the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3150821
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3150799
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

NANO Comparison Summary

Summary:

  • You potentially added 2 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 15
  • DQMHistoTests: Total histograms compared: 15526
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 15526
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 14 files compared)
  • Checked 31 log files, 14 edm output root files, 15 DQM output files

Nano size comparison Summary:

Sample kb/ev ref kb/ev diff kb/ev ev/s/thd ref ev/s/thd diff rate mem/thd ref mem/thd
2500.0 2.447 2.447 0.000 ( +0.0% ) 5.18 5.11 +1.5% 2.154 2.091
2500.001 2.553 2.553 0.000 ( +0.0% ) 4.68 4.61 +1.6% 2.505 2.512
2500.002 2.498 2.498 0.000 ( +0.0% ) 4.78 4.79 -0.2% 2.507 2.480
2500.01 1.241 1.241 0.000 ( +0.0% ) 9.69 9.67 +0.2% 2.221 2.200
2500.011 1.605 1.605 0.000 ( +0.0% ) 5.23 5.18 +1.0% 2.418 2.356
2500.012 1.486 1.486 0.000 ( +0.0% ) 7.49 7.47 +0.2% 2.323 2.330
2500.1 2.161 2.161 0.000 ( +0.0% ) 5.19 5.18 +0.1% 1.993 1.989
2500.2 2.271 2.271 0.000 ( +0.0% ) 6.00 5.97 +0.5% 1.840 1.832
2500.21 1.147 1.147 0.000 ( +0.0% ) 4.38 4.29 +2.0% 2.215 2.215
2500.211 1.505 1.505 0.000 ( +0.0% ) 3.81 3.77 +1.0% 2.122 2.118
2500.3 2.009 2.009 0.000 ( +0.0% ) 12.39 12.27 +0.9% 1.703 1.694
2500.31 1.209 1.209 0.000 ( +0.0% ) 19.33 19.21 +0.6% 2.136 2.118
2500.311 1.586 1.586 0.000 ( +0.0% ) 14.26 13.97 +2.1% 2.005 2.002
2500.4 2.009 2.009 0.000 ( +0.0% ) 12.22 12.31 -0.7% 1.714 1.695

@mandrenguyen
Copy link
Contributor

The nano summary table just above doesn't seem to show any size increase.
Is the change only in mini?
@gmelachr what output size change did you observe in your local tests? With what workflow and how many events?

@gmelachr
Copy link
Contributor Author

gmelachr commented Aug 4, 2023

Let me clarify a bit: I only updated the PhysicsTools/PatAlgos/python/slimming/prunedGenParticles_cfi.py to change-correct the available information in the miniAODs. Using the nominal framework, I was seeing the same production vertex for Kshort and for its daughters. The daughters were stored in the packedGenParticles collection with a wrong production vertex and the Kshort in the prunedGenParticle Collection. Now, the daughters are also stored in the prunedGenParticleCollection with the correct production vertex.

Furthermore, I didn't produce nanoAODs, I produced and checked things only for miniAODs locally.
I produced 655 events for B0->J/ψΚshort->μμππ and 422 events for Λb->J/ψΛ0 ->μμpπ. I checked the size of the miniAODs (with ls -lh) and in both cases (the ones produced with the nominal version and the ones produced with the modified one) the miniAODs had the same size.

Let me know if this responds your question,
Giorgos

@simonepigazzini
Copy link
Contributor

I enabled the nano tests more as a cross-check than anything else. I did not expect a change in size at the NANO level, since the gen level is further pruned at the nano step. I think that overall the impact of the changes is negligible also for MINI

@mandrenguyen
Copy link
Contributor

+reconstruction
Thanks for the explanations.
Changes are acceptable for reco, provided they are for XPOG :-)

@gmelachr
Copy link
Contributor Author

gmelachr commented Aug 7, 2023

Hi,

according to the last message I understand that the changes have been accepted. So, how are we going to proceed now? Are we waiting for a further approval by someone? Furthermore, what is the procedure to backport these changes to 12_4_X?

@perrotta
Copy link
Contributor

perrotta commented Aug 7, 2023

@gmelachr we still miss the review of @cms-sw/xpog-l2 (whch are the main responsible for this code)

@@ -18,7 +18,8 @@
"drop pdgId == 21 && pt < 5", # remove pythia8 garbage
"drop status == 2 && abs(pdgId) == 21", # but remove again gluons in the inheritance chain
"keep abs(pdgId) == 23 || abs(pdgId) == 24 || abs(pdgId) == 25 || abs(pdgId) == 6 || abs(pdgId) == 37 ", # keep VIP(articles)s
"keep abs(pdgId) == 310 && abs(eta) < 2.5 && pt > 1 ", # keep K0
"keep++ abs(pdgId) == 310 || abs(pdgId) == 3122", # Keep Kshort, Lambda0 and their daughters
"keep+ abs(pdgId) == 310 || abs(pdgId) == 3122", # but keep first daughter, to allow lifetime determinations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a problem to leave at least the eta restriction at 2.5?

Can you try with this syntax: "keep+ (abs(pdgId) == 310 || abs(pdgId)==3122) && abs(eta)<2.5 && pt>1`

And check that it still works for you. You can also split Kshort and Lambda0 into two statements adding the eta and pt cuts to both

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I want to store all Kshorts and Lambdas for B0->X(μμ)Kshort(ππ), Β+ ->Χ(μμ)Κ*+ -->μμKshortπ-->μμπππ and Λb->Χ(mumu)Λ0(pπ) in order to measure acceptance. So, a possible cut for eta or for pT would affect the acceptance, wouldn't it?

What is the problem with the current syntax?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok understood. There is no problem, I was just trying to get to the minimal set of particles to add.

@simonepigazzini
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2023

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

@simonepigazzini
Copy link
Contributor

@cms-sw/pdmv-l2

my understanding is that the change is needed in 12_4_X to patch the ongoing production. If you agree to produce the required samples with a dedicated central production we can proceed with a backport to 12_4_X.

@perrotta
Copy link
Contributor

perrotta commented Aug 7, 2023

+1

@cmsbuild cmsbuild merged commit 87715dc into cms-sw:master Aug 7, 2023
@gmelachr
Copy link
Contributor Author

gmelachr commented Aug 7, 2023

@cms-sw/pdmv-l2

my understanding is that the change is needed in 12_4_X to patch the ongoing production. If you agree to produce the required samples with a dedicated central production we can proceed with a backport to 12_4_X.

The productions for the above-mentioned decays have not been requested yet with central productions. I was waiting for the changed to be integrated in CMSSW in order to avoid double productions and possible problems.

The current MC requests for BPAG are running on CMSSW_12_4_11_path3, so please proceed with the backport in order to proceed with the central production too.

thanks

@sunilUIET
Copy link
Contributor

@simonepigazzini we will produce the samples as requested by pwgs. It is fine to backport it.

@simonepigazzini
Copy link
Contributor

@gmelachr, please proceed with the backport. You just need to apply the same changes starting from CMSSW_12_4_11_path3 instead of a recent IB.

Thank you

@gmelachr
Copy link
Contributor Author

gmelachr commented Aug 7, 2023

@gmelachr, please proceed with the backport. You just need to apply the same changes starting from CMSSW_12_4_11_path3 instead of a recent IB.

Thank you

@simonepigazzini And then I will create a pull request to CMSSW_12_4_X, right?

@simonepigazzini
Copy link
Contributor

yep

@perrotta
Copy link
Contributor

perrotta commented Aug 7, 2023

@simonepigazzini @gmelachr what about backporting it in the intermediate releases? Is this something that could be potentially useful in the analysis of this year data (i.e. 13_0_X) as well? Then 13_1 is for Phase2, and 13_2 for HI, maybe not needed there but think about.

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.

6 participants