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.

Sign up for GitHub

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

Jump to bottom

Update TrackingNtuple #21148

Merged
merged 24 commits into from
Nov 8, 2017
Merged

Update TrackingNtuple #21148

merged 24 commits into from
Nov 8, 2017

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Nov 3, 2017

This PR makes the following updates to the TrackingNtuple:

  • add alternative track-to-TrackingParticle matchings
    • best matching TrackingParticle (also for those tracks that would be fake by the standard matching)
      • defined as the TrackingParticle with most SimHits matched to the clusters of the track, at minimum 3 hits of a track must be matched to the TP
    • best matching TrackingParticle starting from the first hit of the track
      • same definition as above but starting from the first hit of the track
  • add various track-to-TrackingParticle matching scores
    • for both "best TP" and "best TP from first hit"
      • shared cluster fraction (the quantity used in QuickTrackAssociatorByHits; of these only this one is added for seeds)
      • shared cluster fraction with the TrackingParticle::numberOfTrackerHits() as the denominator
      • shared cluster fraction with the number of reco clusters matched to the TrackingParticle (as given by ClusterTPAssociation)
      • chi2 calculated from the track and TrackingParticle parameters and the track parameter uncertanties (as in TrackAssociatorByChi2)
    • for each matched TP: the chi2 (as above)
    • tracks: number of invalid hits (inside track and also "inner" and "outer"); this implied a change of branch name trk_nInvalid => trk_nLost
  • include on-the-fly created matched strip hits
    • omitting these this was actually a serious bug causing memory over writes
  • other additions
    • tracks and seeds: number of clusters (mostly because it is used in the track-to-TP matching)
    • TrackingParticles: numberOfTrackerHits(), and the number of reco clusters matched to the TP (as in ClusterTPAssociation)

and in addition:

  • remove unused code from TrackAssociatorByChi2Impl

Tested in 9_4_0_pre3, no changes expected in standard workflows.

@VinInn @ebrondol @hajohajo

@makortel
Add best matched TP, shared hit fractions to track, number of reco hi…
d679ecf 
…ts for TP

Number of reco hits for TP as implemented here is not interesting. In
general the can be many clusters for a given SimHit, while my purpose
was to filter out those SimHits that are not reconstructed. I guess
this is the time to incorporate TrackerHitAssociator.
@makortel
Record all matching SimHits for a cluster instead of the first one
6e00544
@makortel
Save TP::numberOfTrackerHits() and use that for 'SimDenom'
b81ee95 
I'm giving up on calculation of sensible "TP hits matched to
clusters". TP SimHits need to be filtered (to ignore e.g. delta rays),
and they are not generally available without running MixingModule in
playback mode, which is something I'd prefer to not to introduce for
e.g. trackingOnly workflows (since it's much easier to run pileup
samples without the playback mode).
@makortel
Add best TP match also to seeds
980eccd
@makortel
Add a track-TP matching score starting from the first hit of the track
9e9c72f
@makortel
Count matched hits and clusters separately
7269ed9 
Some hits are associated to multiple clustes (e.g. strip matched
hits). For those cases, the reco denominator needs the count of hits,
while the sim denominator needs the count of clusters.
@makortel
Add best TP match from first hit also to seeds
f01af46
@makortel
In case of multiple matches with same number of hits, select the one …
e5094b5 
…with innermost reco hit
@makortel
Switch bestMatchingTrackingParticle() to use the stored index
c553e6b
@makortel
Exclude also SimHits if TrackingParticles are excluded
eece559
@makortel
Remove unused code
1eed581
@makortel
Exclude non-input TrackingParticles from the best match calculation
24e6cb1
@makortel
Remove dead code from TrackAssociaterByChi2Impl
0f0e88e 
They were essentially repeat from getChi2()...
@makortel
Rename getChi2() to trackAssociationChi2() and move to SimTracker/Tra…
9368f70 
…ckAssociation
@makortel
Add track-TP association chi2 to TrackingNtuple
b4593b3
@makortel
Add association chi2 to printout
ac91d91
@makortel
Make it explicit that trackAssociationChi2() returns normalized chi2
575db1a
@makortel
Add back number of clusters associated to a TrackingParticle
6aeb3d8
@makortel
Remove shared hit fractions with sim denominator for seeds as they ar…
a066bfd 
…e not interesting

Seeding aim is not to find all hits of a TP, so these ratios are not interesting.
@makortel
Fix TrackingNtuple to include on-the-fly generated matched strip hits
2c8920b 
Not including them was actually a serious bug causing memory overwrites...
@makortel
Add protections for collection sizes to printTrackingNtuple
a838286
@makortel
Save numbers of inactive hits
1db85ba
@makortel
Use Path instead of EndPath
868f3ab
@cmsbuild cmsbuild added this to the CMSSW_10_0_X milestone Nov 3, 2017
@makortel
Copy link
Contributor Author

makortel commented Nov 3, 2017

So I forgot to add the file... Included now and squashed in the commit where it should have been included.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2017

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21148/1792

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2017

Pull request #21148 was updated. @civanch, @vazzolini, @kmaeshima, @mdhildreth, @dmitrijus, @cmsbuild, @vanbesien can you please check and sign again.

@makortel
Copy link
Contributor Author

makortel commented Nov 3, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2017 •
edited
Loading

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24154/console Started: 2017/11/03 15:01

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2017

+1
Tested at: 4f98c64
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21148/24154/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
2b5a412
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21148/24154/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21148/24154/git-merge-result

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2017

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2900266
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2900093
  • DQMHistoTests: Total skipped: 171
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@civanch
Copy link
Contributor

civanch commented Nov 4, 2017

+1

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2017

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit a584e60 into cms-sw:master Nov 8, 2017
@makortel makortel deleted the ntuple_matchScore branch February 12, 2018 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
code-checks-approved comparison-available dqm-approved fully-signed orp-approved simulation-approved tests-approved
Projects
None yet
Milestone
CMSSW_10_0_X
Development

Successfully merging this pull request may close these issues.
5 participants
@makortel @cmsbuild @civanch @dmitrijus @davidlange6