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

[TRK POG Validation] add fakes for monitoring w/ |eta| > 2.7 #28569

Merged
merged 1 commit into from Dec 18, 2019

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented Dec 6, 2019

PR description:

it turned out that we were missing the --most important-- plots about the fakes in this phase-space

this PR fixes the issue, adding fakes for monitoring w/ |eta| > 2.7
in addition, it adds this monitoring to the trackingOnly sequence as well

add fakes for monitoring w/ |eta| > 2.7 and add this monitoring to the trackingOnly as well

fix

partially fix input track collection
@mtosi
Copy link
Contributor Author

mtosi commented Dec 6, 2019

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28569/13061

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3847/console Started: 2019/12/06 16:37

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

A new Pull Request was created by @mtosi (mia tosi) for master.

It involves the following packages:

Validation/RecoTrack

@andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @wmtford, @ebrondol, @dgulhan 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

cmsbuild commented Dec 6, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fdf3ba/3847/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2794252
  • DQMHistoTests: Total failures: 99
  • DQMHistoTests: Total nulls: 80
  • DQMHistoTests: Total successes: 2793732
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 20744.136 KiB( 33 files compared)
  • DQMHistoSizes: changed ( 20034.0,... ): 5186.034 KiB Tracking/TrackTPEtaGreater2p7
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 9, 2019

@mtosi Please add subsystem name to the PR title for easier identification

@mtosi mtosi changed the title add fakes for monitoring w/ |eta| > 2.7 [TRK POG Validation] add fakes for monitoring w/ |eta| > 2.7 Dec 9, 2019
@mtosi
Copy link
Contributor Author

mtosi commented Dec 9, 2019

thanks @jfernan2 for the comment (and sorry for not having done it before)

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 9, 2019

Thanks @mtosi
However, this PR goes in the opposite direction to what we discussed on the Tracking DQM Sequence review, around 5k plots are being added, some of them (1500) are empty[1]. Can we expect some DQM Tracking clean up at some point?

[1]
cutsRecoMuonSeededStepInOutHp_trackingParticleRecoAsssociation
cutsRecoMuonSeededStepInOut_trackingParticleRecoAsssociation
cutsRecoMuonSeededStepOutInHp_trackingParticleRecoAsssociation
cutsRecoMuonSeededStepOutIn_trackingParticleRecoAsssociation
cutsRecoPixelPairStepHp_trackingParticleRecoAsssociation
cutsRecoPixelPairStep_trackingParticleRecoAsssociation

@mtosi
Copy link
Contributor Author

mtosi commented Dec 9, 2019

this PR is a bug fix

@fabiocos
Copy link
Contributor

fabiocos commented Dec 9, 2019

@mtosi adding several MBs of plots look something strong for a bug fix... Adding MBs of empty plot is something to be clarified IMHO

@mtosi
Copy link
Contributor Author

mtosi commented Dec 9, 2019

well, among the quoted list of directories

cutsRecoMuonSeededStepInOutHp_trackingParticleRecoAsssociation
cutsRecoMuonSeededStepInOut_trackingParticleRecoAsssociation
cutsRecoMuonSeededStepOutInHp_trackingParticleRecoAsssociation
cutsRecoMuonSeededStepOutIn_trackingParticleRecoAsssociation
cutsRecoPixelPairStepHp_trackingParticleRecoAsssociation
cutsRecoPixelPairStep_trackingParticleRecoAsssociation

they are not empty if you are on enough statistics, even just 1k events w/o PU on available relval sample in 11_0_0_pre13, for instance

the added plots, are the one related to the fakes and duplicates which were missing

this plots are particularly important in this phase of validation of the tracker layout and material budget in the high pseudorapidity

@emiglior @mmusich

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 9, 2019

@mtosi Nobody is saying these plots are not important but, can you share your validation of the PR where the plots in those folders get some entries beyond more than 10 events per workflow as the cmsbuild does?
Nevertheless, my question still holds: Can we expect some DQM Tracking clean up at some point?
And the following comment goes beyond the scope of this PR but I take the opportunity so here it goes anyway: We need also some words on how Tracking is performing validation, or if you prefer who and how is looking at 35k (+5k introduced by this PR) plots for Validation?
The twiki presented at the meeting seems not up to date in this respect: https://twiki.cern.ch/twiki/bin/viewauth/CMS/TrackingValidationMC

@cmsbuild cmsbuild modified the milestones: CMSSW_11_1_X, CMSSW_11_0_X Dec 10, 2019
@mmusich
Copy link
Contributor

mmusich commented Dec 16, 2019

Hello, out of curiosity, what is the plan for integration of this PR?
Is it subject to further discussion on the amount of plots in the tracking workspace?
As for:

can you share your validation of the PR where the plots in those folders get some entries beyond more than 10 events per workflow as the cmsbuild does?

@mtosi can you post here the plots you circulated in private thread demonstrating their utility?
As pointed out by Mia, this PR has implications on the Phase2 Tracker layout decisions, though these plots can be run by hand it would be useful to include them in regular release validation.

@mtosi
Copy link
Contributor Author

mtosi commented Dec 17, 2019

apologies for the delay
looking at the summary plot w/ the statistics of reconstructed tracks in the different categories (which are the sub-directories) you clearly see that plots should not be empty
image

@jfernan2
Copy link
Contributor

@mtosi sorry but the plot you link is very similar to the one produced by cmsbuild in the PR test for wf23234 (ttbar): https://tinyurl.com/rz5eqaz
image
with indeed two orders of magnitude less events and some population in the MuonSeeded collections but still having most of the plots with no entries in the folders from the directories quoted above in previous comments. Or in some cases with a few entries but all lying in under or overflow bins, so I suggest considering a binning check in this new set of 5k plots to make them actually useful.

Nevertheless, I understand this is a matter of muon tracks but there is no RelValZMM sample checked in this test for Phase2.

So. I am signing the PR since I understand this is needed, however with this addition the total Tracking account of MEs in RelVal samples is ~40k. Hence I urge you to reconsider a revision of the Tracking Validation procedure for the next year which, as I said before, was not presented in the DQM Sequence review. Thank you

@jfernan2
Copy link
Contributor

+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 now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@mtosi
Copy link
Contributor Author

mtosi commented Dec 17, 2019

@jfernan2 thanks !

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 22319db into cms-sw:master Dec 18, 2019
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

5 participants