Overlap recheck #118

osschar · 2023-02-24T16:15:28Z

Recheck overlap hit chi2 after the update with the primary hit.

Use the new chi2 to select wich overlap hits to append to the candidate, for the moment still not either adding the overlap chi2 to the cand score nor using the overlap to update the parameters. These could both be considered in the future but would require even further tuning of the scoring function.

Post-update chi2 distribution has a horrendous tail up to 10^7.

MTV plots: http://xrd-cache-1.t2.ucsd.edu/matevz/PKF/overlap-recheck-1/

chi2-60: about 35% of overlap hits are kept;
chi2-1100: 66%;
chi2-10000: 90%.

slava77 · 2023-06-02T16:15:49Z

related to this and true hits: @leonardogiannini windows tuning algorithm uses truth.
Perhaps the first step would be to check that if both hits in the layer are true that the post-update chi2 is well-behaved (follows the chi2 distribution).

slava77 · 2023-06-07T12:54:19Z

@osschar
it looks like this PR has a conflict now in RecoTracker/MkFitCMS/standalone/Makefile
Please rebase.

slava77 · 2023-06-07T15:42:27Z

MTV plots: http://xrd-cache-1.t2.ucsd.edu/matevz/PKF/overlap-recheck-1/
* chi2-60: about 35% of overlap hits are kept;

* chi2-1100: 66%;

* chi2-10000: 90%.

In the initial step built tracks I see

So, it looks like in all the variants we are actually adding hits; even more layers.
I guess I'm better off to trace/debug this recheck logic, as I couldn't quite follow the code steps.

osschar · 2023-06-07T16:40:12Z

The conflict has been cleared ... note that I force-pushed the rebased branch back so please delete any local copies before re-checkouting.

slava77 · 2023-06-07T21:22:06Z

The conflict has been cleared

🍾

slava77 · 2023-06-08T13:48:59Z

RecoTracker/MkFitCore/src/MkBuilder.cc

+
+          // XXXX For now we DO NOT use chi2 as this was how things were done before the post-update
+          // chi2 check. To use it we should retune scoring function (might be even simpler).
+          if (mkfndr->m_FailFlag[fi] == 0 && mkfndr->m_Chi2[fi] >= 0.0f && mkfndr->m_Chi2[fi] <= 60.0f) {


I'd like to propose to make this configurable. We have chi2CutOverlap parameter in iter config already, which is currently unused; and I guess it can be reused here.
the logic could probably be

Suggested change

if (mkfndr->m_FailFlag[fi] == 0 && mkfndr->m_Chi2[fi] >= 0.0f && mkfndr->m_Chi2[fi] <= 60.0f) {

if (m_iteration_params->chi2CutOverlap < 0 || (mkfndr->m_FailFlag[fi] == 0 && mkfndr->m_Chi2[fi] >= 0.0f && mkfndr->m_Chi2[fi] <= m_iteration_params->chi2CutOverlap)) {

the default could be set to -1 in all iterations jsons for now

We also have pT cut for overlap ... it used to be applied so only tacks with pT > 1 GeV would pick up overlaps.

Default in jsons now seem to be 3.5 for chi2 and 0 for pT ... neither of them being used now.

Do you want me to just commit the above suggestion for now then?

uhm, pTCutOverlap is used in RecoTracker/MkFitCore/src/CandCloner.cc; it does nothing because the value is zero. I can see it could become useful/reconsidered to change from 0 and pt being relatively well defined, I think it's fine where it is.

I started testing a case with

tc.addHitIdx(seed_cand_overlap_idx[ii].ovlp_idx, curr_layer, mkfndr->m_Chi2[fi]);

but now I realize that it's a bad idea: we do not have a candidate without the overlap hit, if the overlap passes the chi2 cut.

I guess a more meaningful option is to compute the score with and without the overlap hit and then decide to add the hit or not.

I guess a more meaningful option is to compute the score with and without the overlap hit and then decide to add the hit or not.

is there a clean way to pop a hit from a TrackCand ?

Adding of main / overlap hit as combinatorics is something I was thinking about in the context of thin / thick layers. I will check how we could hack this in, part of it would have to be done beforehand, in CandCloner, as you also said.

It's not really easy to pop a hit as there is a common tree-over-vector representation of hits for all Cands under one CombCandidate.

All this goes into the direction I'm thinking about things ... having some intermediate thick layer intermediate candidate.

It's not really easy to pop a hit as there is a common tree-over-vector representation of hits for all Cands under one CombCandidate.

can't I just set lastHitIdx_ = refLastHoTNode().m_prev_idx and update n found, n overlap and chi2 in the TrackCand, without touching the HoT nodes in CombCandidate?
or is there some code that would find an orphaned HoT node and complain?

No, you're right this should work for the last hit -- it will just leave a slot unused as max-used-slot-id is kept in CombCandidate. But you want to keep two, right ... so it will be still used by the cand that has both hits.

There is recompactification done before backward search and there only the hits from the best cand get repopulated ... erasing all the others, including the void slots.

slava77 · 2023-06-09T13:35:03Z

Here is what I see for the pixelLess where all other iterations are using CKF

Compared to the initialStep from the initial PR tests

So, at least in these iterations the impact is different: initialStep gets worse and pixelLess gets better. Just an extra motivation to make this overlap recheck feature configurable.

IHateLinus · 2023-06-09T15:01:12Z

This is the largest positive change in pixel less we’ve, I think..

…

On Jun 9, 2023, at 6:35 AM, Slava Krutelyov ***@***.***> wrote: Here is what I see for the pixelLess where all other iterations are using CKF <http://uaf-10.t2.ucsd.edu/~slava77/figs/mic/CMSSW_12_6_0_pre5-pixmkf/RelValTTbar_14TeV-PU_2022_realistic/mtv-pixmkf-orig-prune1st-overlap-recheck-50e3c1a/plots_pixelLessStep.html> <https://urldefense.com/v3/__https://user-images.githubusercontent.com/4676718/244705915-b8451022-2fe8-4638-a49c-c1c74dc3733a.png__;!!Mih3wA!HLNeWSwPovIkBeZDqHfg45OsTMq4DvXakQjSOFyNPpGPEIn8sGZe7bor7pWWsbqQPfYgC0CTZWQWf40_pPBbB2A$> Compared to the initialStep from the initial PR tests <http://xrd-cache-1.t2.ucsd.edu/matevz/PKF/overlap-recheck-1/plots_initialStep.html> <https://urldefense.com/v3/__https://user-images.githubusercontent.com/4676718/244706586-45f931bf-0d55-4e21-ba8c-8d9c6f0b080d.png__;!!Mih3wA!HLNeWSwPovIkBeZDqHfg45OsTMq4DvXakQjSOFyNPpGPEIn8sGZe7bor7pWWsbqQPfYgC0CTZWQWf40_GXHFCuM$> So, at least in these iterations the impact is different: initialStep gets worse and pixelLess gets better. Just an extra motivation to make this overlap recheck feature configurable. — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/trackreco/cmssw/pull/118*issuecomment-1584587332__;Iw!!Mih3wA!HLNeWSwPovIkBeZDqHfg45OsTMq4DvXakQjSOFyNPpGPEIn8sGZe7bor7pWWsbqQPfYgC0CTZWQWf40_5vjJNjo$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABDVH4NKBN4JYQBH3S2D7CLXKMRBFANCNFSM6AAAAAAVHCPBD4__;!!Mih3wA!HLNeWSwPovIkBeZDqHfg45OsTMq4DvXakQjSOFyNPpGPEIn8sGZe7bor7pWWsbqQPfYgC0CTZWQWf40_HmJF6-s$>. You are receiving this because you are subscribed to this thread.

slava77 · 2023-12-12T15:59:50Z

there is apparently a conflict now in RecoTracker/MkFitCore/src/PropagationMPlex.icc

I'm not sure though how recent this is.

After the conflict is resolved before submitting a PR to cmssw it would be good to run the performance tests and see no changes in mkFit-5 setup and an increase in eff in the mkFit5+pixelLess.
@leonardogiannini are you available to run it?

slava77 · 2023-12-12T16:43:50Z

still

This branch cannot be rebased due to conflicts

osschar · 2023-12-12T16:56:55Z

You want to update the trackreco/master and then I do the rebase?
I can also do some squashing.
Do we also need code-format etc?

slava77 · 2023-12-12T17:24:20Z

You want to update the trackreco/master and then I do the rebase? I can also do some squashing.

The complaint about conflicts is supposedly relative to the trackreco/master.
I usually rebase;
merging the master in the topic branch at least in the past was leading to explosion of differences.

Do we also need code-format etc?

yes, it will be needed in the cmssw pr, so why not do it before the submission

…iven layer.

into which update-list to put a hit with an overlap.

osschar · 2023-12-12T18:39:53Z

Force-pushed the rebased version ... remove local branch before pulling.
I can also do squash later.

slava77 · 2023-12-12T19:27:36Z

Force-pushed the rebased version

looks green now
🍾

slava77 · 2023-12-12T19:29:50Z

I can also do squash later.

some commits from me are useless, could be combined to 1 or two. Some others may be more useful since they are scattered over many files

leonardogiannini · 2023-12-13T21:02:38Z

@slava77

Here is the comparison (both 1K tt and 100 events single thread for timing)

http://uaf-10.t2.ucsd.edu/~legianni/PR118/

Looks as expected

Let me know if you also want to cross-check the old pixelLess with no recheck

slava77 · 2023-12-13T21:31:11Z

Let me know if you also want to cross-check the old pixelLess with no recheck

I think we also need a sanity check to show that mkfit5 does not change from this PR.

Then indeed, on your set already provided it would be good to add the pixelLess before this PR.

slava77 · 2023-12-13T21:34:24Z

all tracks OOB reminds me that for this specific update it would be very informative to show plots with pt< 0.9 GeV (instead of pt> 0.9 GeV).

Last year I used the following:

from HLTrigger.Configuration.common import producers_by_type
for m in producers_by_type(process, "MultiTrackValidator"):
  ma = m.histoProducerAlgoBlock
  for b in [ma.TpSelectorForEfficiencyVsEta, ma.TpSelectorForEfficiencyVsPhi, ma.TpSelectorForEfficiencyVsPt, ma.TpSelectorForEfficiencyVsVTXR, ma.TpSelectorForEfficiencyVsVTXZ, ma.generalTpSelector]:
	b.ptMin = 0.05
	b.ptMax = 0.9

osschar · 2023-12-14T16:52:12Z

@osschar can the new layers be enabled by iteration or is it a global thing? I wanted to see if pixelLess gets better with that even out of the box

What do you mean with new layers? You mean add layers to traverse in each region? If that, then yes, you can modify it, it's part of the layer plan. But if you are adding them at the beginning then one needs to probably also fix the pick-up layers etc. ... I'd have to review the code to remember how it's done now, I did change this a bit during backwards-search / seed-reg rebuild implementation.

slava77 · 2023-12-14T17:03:04Z

What do you mean with new layers?

I mean the new layer processing (the one that was integrated but disabled)

leonardogiannini · 2023-12-14T17:06:54Z

the cross-check for low pT pixelLess is here http://uaf-10.t2.ucsd.edu/~legianni/PR118/plots_lowpt_xcheck/

(original is bad name choice, please ignore it)

slava77 · 2023-12-14T17:11:34Z

the cross-check for low pT pixelLess is here http://uaf-10.t2.ucsd.edu/~legianni/PR118/plots_lowpt_xcheck/

Thank you.
How many events is this?
Also, which release and input dataset is this?

slava77 · 2023-12-14T17:17:50Z

I wanted to ask to add the low pt displaced, e.g. from /RelValXiMinus_13p6TeV/CMSSW_13_3_0-133X_mcRun3_2023_realistic_v3-v1/GEN-SIM-DIGI-RAW

this is no PU, so, could process at least 10K events (if not the full sample; no timing necessary, so loading multiple threads will be OK)

slava77 · 2023-12-14T17:24:23Z

the cross-check for low pT pixelLess is here http://uaf-10.t2.ucsd.edu/~legianni/PR118/plots_lowpt_xcheck/

e.g. OOB high purity dist of all tracks

oddly enough a similar loss is not visible in the efficiency plot (which is using signal TPs); unclear if the other cuts applied of efficiency plots play some role.

slava77 · 2023-12-14T17:29:00Z

e.g. OOB high purity dist of all tracks

and pixelLess by itself high purity (by orig algo)

by this measure there is still 10-15% loss with dxy > 1 cm

osschar · 2023-12-14T17:33:03Z

What do you mean with new layers?

I mean the new layer processing (the one that was integrated but disabled)

Ah, good idea :) Let me take a look ... in principle we just need to call selectHitIndicesV2() from MkBuilder if some new flag is set. If this turns out to help, I can also try to optimize it, at least to the 0-th order.

So, the plan:
0. I recheck what selectHitIndicesV2() does now, see what tuning is potentially needed;

I add a new bool to IterationParams, m_use_hit_selection_v2; use it in find_tracks()
set it to true for pixelLess in JSONs
?

slava77 · 2023-12-14T17:34:29Z

So, the plan: 0. I recheck what selectHitIndicesV2() does now, see what tuning is potentially needed;
1. I add a new bool to IterationParams, m_use_hit_selection_v2; use it in find_tracks()

2. set it to true for pixelLess in JSONs
   ?

"true" only if it helps

osschar · 2023-12-14T17:36:18Z

hehe, yes, sure ... but we have to try it first and I have to update all the jsons anyway to have the new member.

leonardogiannini · 2023-12-14T18:04:42Z

I wanted to ask to add the low pt displaced, e.g. from /RelValXiMinus_13p6TeV/CMSSW_13_3_0-133X_mcRun3_2023_realistic_v3-v1/GEN-SIM-DIGI-RAW

this is no PU, so, could process at least 10K events (if not the full sample; no timing necessary, so loading multiple threads will be OK)

All the plots so far are using 1k ttbar. I'm going to run on the XiMinus.

The release is cmssw_14_0_0_pre1, so it should include all the developments

slava77 · 2023-12-14T18:15:56Z

All the plots so far are using 1k ttbar.

which release of ttbar, just for a reference?

I'm going to run on the XiMinus.

The release is cmssw_14_0_0_pre1, so it should include all the developments

OK, good.

osschar · 2023-12-14T18:18:49Z

For testing, should I just enable v2 for forward search? we should have relatively good precision for backwards search, no?

leonardogiannini · 2023-12-14T18:21:21Z

All the plots so far are using 1k ttbar.

which release of ttbar, just for a reference?

I'm going to run on the XiMinus.
The release is cmssw_14_0_0_pre1, so it should include all the developments

OK, good.

ttbar is quite old.. probably 12_5_x but I am not aware of changes in step2 after the beamspot was update. I can use a new relval too if you think it necessary

slava77 · 2023-12-14T19:41:06Z

ttbar is quite old.. probably 12_5_x but I am not aware of changes in step2 after the beamspot was update. I can use a new relval too if you think it necessary

is the beamspot payload in the GT used consistently with the SIM?

slava77 · 2023-12-14T19:45:18Z

For testing, should I just enable v2 for forward search? we should have relatively good precision for backwards search, no?

I wouldn't be so sure for backward: it's complicated for eta>1.5 where pixel-less has to go back from TID/TEC to FPIX where distances can be around 1 m (and the pt is pretty low).

…selectHitIndicesV2().

osschar · 2023-12-14T20:44:02Z

OK, set it to true for pixelLess, both direction.
@slava77 please check that q-half-length calculation makes sense, this is used as part of pre-selection and it was only done for strips (0.5 * sqrt(12) -> sqrt(3)), I just did 3*sigma for pixels:
https://github.com/trackreco/cmssw/pull/118/files#diff-bfae47efa0a65c46e3d9903010963a21d4a2d0fa5eefc606c9ace1749f4c518dR174

slava77 · 2023-12-14T23:50:37Z

I just did 3*sigma for pixels

it's not going to give a length, but it doesn't matter for pixels, fwiw this value is not distinguishable from 0 for pixels in this context of hit selections

osschar · 2023-12-15T02:19:08Z

Oh, you mean this does not represent the true sigma in z (barrel) r (endcap) direction for pixels? It is used here as hit compatibility check, with extra factor of 1.2:
https://github.com/trackreco/cmssw/blob/overlap-recheck/RecoTracker/MkFitCore/src/MkFinder.cc#L985

slava77 · 2023-12-15T02:32:50Z

Oh, you mean this does not represent the true sigma in z (barrel) r (endcap) direction for pixels? It is used here as hit compatibility check, with extra factor of 1.2: https://github.com/trackreco/cmssw/blob/overlap-recheck/RecoTracker/MkFitCore/src/MkFinder.cc#L985

is new_ddq = std::abs(new_q - L.hit_q(hi)); propagated vs actual hit? It looks like new_ddq < 1.2 * L.hit_q_half_length(hi) will always fail for at least the first one (and likely 2) pixel hits on the way from TID/TEC.

slava77 · 2023-12-15T02:34:50Z

is new_ddq = std::abs(new_q - L.hit_q(hi)); propagated vs actual hit? It looks like new_ddq < 1.2 * L.hit_q_half_length(hi) will always fail for at least the first one (and likely 2) pixel hits on the way from TID/TEC.

the test should add the propagation uncertainty or some other parametric tolerance representing expected propagation uncertainty.

slava77 · 2023-12-15T02:38:14Z

it seems like selV2 as it is now should only be in the forward direction for pixelLess

slava77 · 2023-12-15T02:46:26Z

it seems like selV2 as it is now should only be in the forward direction for pixelLess

now I can't recall where the hit overflow was happening at a worse rate in the pixelLess (forward or backward) ... hopefully mostly forward where selV2 logic related to the hit size would still make sense.

…search and for doing hit preselection.

osschar · 2023-12-15T12:18:50Z

is new_ddq = std::abs(new_q - L.hit_q(hi)); propagated vs actual hit? It looks like new_ddq < 1.2 * L.hit_q_half_length(hi) will always fail for at least the first one (and likely 2) pixel hits on the way from TID/TEC.

the test should add the propagation uncertainty or some other parametric tolerance representing expected propagation uncertainty.

OK, I pushed this addition. The extra factors we have hardcoded there could now probably be reduced.

slava77 · 2023-12-15T13:24:22Z

the test should add the propagation uncertainty or some other parametric tolerance representing expected propagation uncertainty.

OK, I pushed this addition. The extra factors we have hardcoded there could now probably be reduced.

Just to be sure, is it the error after propagating to the nominal radius of a given layer?

osschar · 2023-12-15T14:23:52Z

yes, layer center r or z.

osschar force-pushed the overlap-recheck branch from 5cfa541 to a165e21 Compare June 7, 2023 16:34

slava77 reviewed Jun 8, 2023

View reviewed changes

slava77 mentioned this pull request Dec 8, 2023

Overlap recheck with score check #141

Merged

osschar mentioned this pull request Dec 12, 2023

Add values for new member bool IterationParams::recheckOverlap. cms-data/RecoTracker-MkFit#13

Merged

osschar mentioned this pull request Dec 12, 2023

Add values for new member bool IterationParams::recheckOverlap. trackreco/RecoTracker-MkFit#2

Merged

osschar and others added 7 commits December 12, 2023 10:09

[MkFit] Recheck overlap hit chi2 after update with primary hit on a g…

5016abe

…iven layer.

Add note to mkfit label meaning description.

46fac21

enable post-update chi2 cut at 60

9bb5765

add overlap hit chi2 to the score

7355645

sidestep the overlap hit if it decreases the score

63b5bd4

Add bool IterationParams::recheckOverlap, use it in CandCloner to decide

8e964ec

into which update-list to put a hit with an overlap.

Update to track_scorer func, code-checks, code-format.

0e07d81

osschar force-pushed the overlap-recheck branch from 31f712a to 0e07d81 Compare December 12, 2023 18:39

osschar added 2 commits December 14, 2023 12:28

Add bool IterationParams::useHitSelectionV2, when true use MkFinder::…

fc82e45

…selectHitIndicesV2().

Rescale pixel hit sigma by 3 to get to cluster extents in q direction.

bce91dd

In MkFinder::selectHitIndicesV2() use track errors for extending bin …

a2c01cc

…search and for doing hit preselection.

osschar merged commit 57114d1 into master Dec 18, 2023

	if (mkfndr->m_FailFlag[fi] == 0 && mkfndr->m_Chi2[fi] >= 0.0f && mkfndr->m_Chi2[fi] <= 60.0f) {
	if (m_iteration_params->chi2CutOverlap < 0 \|\| (mkfndr->m_FailFlag[fi] == 0 && mkfndr->m_Chi2[fi] >= 0.0f && mkfndr->m_Chi2[fi] <= m_iteration_params->chi2CutOverlap)) {

Overlap recheck #118

Overlap recheck #118

Conversation

osschar commented Feb 24, 2023 • edited Loading

slava77 commented Jun 2, 2023

slava77 commented Jun 7, 2023

slava77 commented Jun 7, 2023

osschar commented Jun 7, 2023

slava77 commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Jun 9, 2023

IHateLinus commented Jun 9, 2023 via email

slava77 commented Dec 12, 2023

slava77 commented Dec 12, 2023

osschar commented Dec 12, 2023

slava77 commented Dec 12, 2023

osschar commented Dec 12, 2023

slava77 commented Dec 12, 2023

slava77 commented Dec 12, 2023

leonardogiannini commented Dec 13, 2023

slava77 commented Dec 13, 2023

slava77 commented Dec 13, 2023

osschar commented Dec 14, 2023

slava77 commented Dec 14, 2023

leonardogiannini commented Dec 14, 2023

slava77 commented Dec 14, 2023

slava77 commented Dec 14, 2023

slava77 commented Dec 14, 2023

slava77 commented Dec 14, 2023

osschar commented Dec 14, 2023

slava77 commented Dec 14, 2023

osschar commented Dec 14, 2023

leonardogiannini commented Dec 14, 2023

slava77 commented Dec 14, 2023

osschar commented Dec 14, 2023

leonardogiannini commented Dec 14, 2023

slava77 commented Dec 14, 2023

slava77 commented Dec 14, 2023

osschar commented Dec 14, 2023

slava77 commented Dec 14, 2023

osschar commented Dec 15, 2023

slava77 commented Dec 15, 2023

slava77 commented Dec 15, 2023

slava77 commented Dec 15, 2023

slava77 commented Dec 15, 2023

osschar commented Dec 15, 2023

slava77 commented Dec 15, 2023

osschar commented Dec 15, 2023

osschar commented Feb 24, 2023 •

edited

Loading