Modified CMSSW ranking (seed-based, no conflict)

[mmasciov]

This PR is meant to replace PR #173
Wrt. the latest version of PR #173 this has no conflict with the devel branch, and has no unused parameter carried around.

Benchmark: https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_mod_seedbased_250evts_noconflict/

Benchmark for PR #173 :
https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_mod_seedbased_250evts/

Benchmark for PR #167 :
https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_250evts/

If people agree, we can merge this PR, and close PR #173 .

[mmasciov]

Efficiencies:

• This PR: https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_mod_seedbased_250evts_noconflict/SIMVAL/SKL-SP_CMSSW_TTbar_PU70_eff_eta_build_pt0p0_SIMVAL.png
• PR Candidate ranking à la CMSSW, modified to maximize efficiency and minimize fake rate #173 (same): https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_mod_seedbased_250evts/SIMVAL/SKL-SP_CMSSW_TTbar_PU70_eff_eta_build_pt0p0_SIMVAL.png
• PR Candidate ranking à la CMSSW #167 (last best efficiency, merged): https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_250evts/SIMVAL/SKL-SP_CMSSW_TTbar_PU70_eff_eta_build_pt0p0_SIMVAL.png

[kmcdermo]

@mmasciov Thanks for resolving the conflicts. Go ahead and close #173.

If we merge this as-is, we ought to make an open Issue on what @osschar pointed out at a Friday meeting a couple of weeks ago: namely, the score counting is done at every comparison. It would be nice to see the scoring done once per group of candidates (making a pair of score + cand idx for each candidate), then sort the pairs by score.

[kmcdermo]

Or, as @srlantz and @dan131riley suggest: we use a heap instead, making a priority queue up to N cands to save.

[srlantz]

The bounded priority queue idea was actually implemented by @dan131riley a couple of years ago, in a branch he called dsr-track-queue. Also Matthieu implemented a similar idea for the GPU. At the time, Dan found it didn't seem to make much difference in performance on CPUs. But that finding may change if we get into more complex scoring calculations and pairwise candidate comparisons.

[mmasciov]

I finally managed to get CE to be identical to the previous version, even when I assign scores directly to each candidate.
However, note that the discrepancy was due to the fact that even for CE, it happens that the full candidates (not the CE simpler structs) are ranked. This happens for extra candidates, in MkBuilder.

There's also another call in TTreeValidation, which currently represents an issue, as the objects in TTreeValidation know little of how they got there. For the moment, I didn't touch TTreeValidation.

Also, in the local repository I'm working in, once I obtained what I wanted, I am not able to run the CE SIMVAL on more than 100 events. CMSSWVAL for CE fails, with a (as usually) unclear segmentation violation, even on 100 events.

Finally, let me note one weird thing that I noticed:

• old simval (before assigning score): https://mmasciov.web.cern.ch/mmasciov/fullval_08Nov/old/sim/SKL-SP_CMSSW_TTbar_PU70_eff_eta_build_pt0p0_SIMVAL.png
• new simval:
  https://mmasciov.web.cern.ch/mmasciov/fullval_08Nov/assignedscore/sim/SKL-SP_CMSSW_TTbar_PU70_eff_eta_build_pt0p0_SIMVAL.png

Now, as you can see, green and red points are identical (as we wanted them to be).
Very unexpected is that CMSSW points are moving...? I wonder how this is possible: I thought we would simply read CMSSW tracks.

Maybe any of you (@kmcdermo for instance) can see where the problem is, based on the code changes?

[kmcdermo]

@mmasciov Thanks for the update! This will need a rebase to include the latest changes from Slava (and maybe squash all the indentention changes into one).

I will look more closely at what needs to be done for TTreeValidation. Can you specify which call/line you are referring to?

I suspect the crash in the validation is due to the same issue as before, re: SlurpIn. I can investigate for sure sometime Sunday (just some dumb printouts before/after SlurpIn in BkFit).

As for the shift in CMSSW tracks in SimVal, I do not have a good answer yet. Nothing in the changes jumps out to me. When you say "old simval", which version of the scoring are you referring to?

[mmasciov]

@kmcdermo
I see one call only in TTreeValidation: https://github.com/cerati/mictest/blob/devel/TTreeValidation.cc#L884

For old simval, I'm here comparing to PR #173 (as I had a local copy of that branch). The efficiencies for that PR are identical to PR #182 (before my latest commits). In fact, physics performance should stay identical. Now, it is for STD and CE tracks, but strangely (for me) not for CMSSW.

[kmcdermo]

Ah, I see. So indeed, everything should be identical...

[kmcdermo]

However, something could be strange here with actually the TTreeValidation.cc line you pointed out. Do we write out the score to the tracks after building (i.e. at the very end when we write out the hit list and such)?

If the score is not stored in the track object, then something weird could be happening here with CMSSW (I'd have to think hard about it: tracing methods + maps + sorting, and my brain is over capacity at the moment).

Or, we can just add another method between the building (+ fitting) and the validation to just compute the score for the tracks (inside the validation routines so as not to affect the compute performance).

[osschar]

Hmmh, if sizeof Track is changed, this is no good :) There should be warnings when opening a binary file. If the score is the last member, it will have random value after reading.

I thought you will store scores into an array and sort indices of that array, not Track vectors.

[osschar]

4th answer here:
https://stackoverflow.com/questions/1577475/c-sorting-and-keeping-track-of-indexes

Then you end up with sorted index array and you can copy out elements in the right sequence. In standard, we do the copy from tmp_cands to event-of-combined-candidates[i] ... this loop should replace that copy. I'll have to look at clone engine case.

[mmasciov]

Closing this in favor of PR #186