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 Gen vertex time and position to NanoAOD #32008

Merged
merged 14 commits into from Nov 15, 2020
Merged

Conversation

kdlong
Copy link
Contributor

@kdlong kdlong commented Nov 2, 2020

PR description:

Adds the Gen vertex x,y,z position and time (t0) as a new table in the NanoAOD.

This is needed for an ongoing study in the W mass analysis to determine the rate at which the primary vertex is not correctly assigned with the sum pt^2 algorithm. t0 could be useful for exotics searches. According to @bendavid adding such info to the nano has been discussed briefly in the physics tools workshop planning.

PR validation:

I checked that the info gets added properly to a test MC sample. I just took the precision position from the reco vertex. For t0, I don't know what the needed precision would be.

Would also add t0, but it's not straightforward in the current setup
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32008/19534

@mariadalfonso
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

A new Pull Request was created by @kdlong (Kenneth Long) for master.

It involves the following packages:

PhysicsTools/NanoAOD

@cmsbuild, @santocch, @mariadalfonso, @gouskos, @fgolf can you please review it and eventually sign? Thanks.
@gpetruc, @peruzzim, @swertz this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

+1
Tested at: 9c738f9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-400fee/10452/summary.html
CMSSW: CMSSW_11_2_X_2020-11-02-1100
SCRAM_ARCH: slc7_amd64_gcc820

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-400fee/10452/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-400fee/10452/git-merge-result

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-400fee/10452/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: 35
  • DQMHistoTests: Total histograms compared: 2544092
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2544069
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@cmsbuild
Copy link
Contributor

Pull request #32008 was updated. @cmsbuild, @santocch, @mariadalfonso, @gouskos, @fgolf can you please check and sign again.

@mariadalfonso
Copy link
Contributor

please test workflow 546,547,548

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 13, 2020

The tests are being triggered in jenkins.
Test Parameters:

@peruzzim
Copy link
Contributor

@peruzzim and I discussed this a bit more offline. It would nice if the XYZ and t0 were in the same object, but it seems like their is no 4D class corresponding to the ROOT Point3D class used to store the XYZ. We could make one, but this seems not worth it to me.

Instead, I added the option for the GlobalVariablesTables to be an extension. It is false by default, and should be transparent to all the current workflows. I tested that this works and that it appears in the DQM (and is filled now).

If there is a strong opposition to this, I would just 1) Rename the GenVtx_t0 so it is in it's own table that doesn't clash with the GenVtx table 2) drop t0 for now

@kdlong why don't you use reco::Vertex? It has a simple constructor that takes position, error matrix (you can set it to null) and time.

@bendavid
Copy link
Contributor

@peruzzim the discussion we're having here is exactly the reason xyz and t0 are stored in separate branches in the AOD and MINIAOD in the first place.

We can perhaps change that for future releases, but presumably we can just deal with this fact at the nanoaod level for the moment...

@kdlong
Copy link
Contributor Author

kdlong commented Nov 13, 2020

@peruzzim I really don't think the current solution is less elegant than putting a gen object in a reco class, and given that it's working and I understood the deadline was today, I'd rather not revisit it.

@cmsbuild
Copy link
Contributor

+1
Tested at: 0afeac5
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-400fee/10732/summary.html
CMSSW: CMSSW_11_2_X_2020-11-12-2300
SCRAM_ARCH: slc7_amd64_gcc820

@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-400fee/10732/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-400fee/546.0_DYToLL_M-50_13TeV_pythia8+DYToLL_M-50_13TeV_pythia8+NANOGENFromGen
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-400fee/547.0_DYToll01234Jets_5f_LO_MLM_Madgraph_LHE_13TeV+DYToll01234Jets_5f_LO_MLM_Madgraph_LHE_13TeV+Hadronizer_TuneCP5_13TeV_MLM_5f_max4j_LHE_pythia8+NANOGENFromGen
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-400fee/548.0_TTbar_Pow_LHE_13TeV+TTbar_Pow_LHE_13TeV+Hadronizer_TuneCP5_13TeV_powhegEmissionVeto2p_pythia8+NANOGENFromGen

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: 35
  • DQMHistoTests: Total histograms compared: 2529296
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2529273
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.724 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 1325.7 ): 0.724 KiB Physics/NanoAODDQM
  • Checked 148 log files, 22 edm output root files, 35 DQM output files

@mariadalfonso
Copy link
Contributor

With the last implementation the DQM is finally fixed.
The current solution extend the GlobalVariablesTableProducer along what was suggested here
#32008 (comment)
Eventually things can be improved for nanoV9 following the discussion in the pr

@mariadalfonso
Copy link
Contributor

+xpog

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 49a8fd1 into cms-sw:master Nov 15, 2020
cmsbuild added a commit that referenced this pull request Nov 15, 2020
@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

7 participants