Fixing the disp tracking bug

[gabhijith]

Changed a couple of asserts into if statements

PR description:

This is to fix the bug when using displaced tracking. The problem is coming from a couple of "pile-up" tracks, reconstructed using displaced seed. There is a slight discrepancy arising from the approx projection.

To rectify this, I have turned a couple of asserts in the match calculator to if statements. This way, the function of asserts is served when we have this discrepancy, that if conditions will do the job, without breaking the program.

PR validation:

The fix is run over ttbar+200 PU for validation.

[tomalin]

It's illegal to have unnamed constants hard-wired in code, such as 0.25. Are you able to name these, and perhaps add a comment explaining how the value is derived?

[tomalin]

Could you please check if comments in Settings.h make it clear how to switch between exact & approx floating point. Is it the same cfg param for prompt & displaced tracking? Perhaps useapprox_?

[tomalin]

Compiler says imatch may be undefined at L482. I think it is right! Same true for variable match.

[gabhijith]

Yes, Sorry about that.
I Fixed it,

[tomalin]

Replace "dphi " by " dphi "

[gabhijith]

It's illegal to have unnamed constants hard-wired in code, such as 0.25. Are you able to name these, and perhaps add a comment explaining how the value is derived?

These asserts were in place before. I don't know the exact reason for 0.25 or 0.2

[tomalin]

It's illegal to have unnamed constants hard-wired in code, such as 0.25. Are you able to name these, and perhaps add a comment explaining how the value is derived?

These asserts were in place before. I don't know the exact reason for 0.25 or 0.2

@aryd do you know what motivates numbers 0.2 & 0.25 in MatchCalculator.cc? Displaced tracking code exceeds them, but don't know if it matters.

Since these constants were already in the code, and the authors don't know what they are, they needn't delay this PR. I've added a request to eliminate them to #45 .

[tomalin]

I confirm that displaced tracking no longer crashes. Checked on 1k ttbarPU200.

[gabhijith]

Could you please check if comments in Settings.h make it clear how to switch between exact & approx floating point. Is it the same cfg param for prompt & displaced tracking? Perhaps useapprox_?

I have added the switch to make sure approx turns on with the useapprox_ in settings.h

[aehart]

Could you please check if comments in Settings.h make it clear how to switch between exact & approx floating point. Is it the same cfg param for prompt & displaced tracking? Perhaps useapprox_?

I have added the switch to make sure approx turns on with the useapprox_ in settings.h

I'm not sure this is what we want.

Previously in the TrackletCalculatorDisplaced, we were assigning the exact floating point parameters and projections to all three sets of values in the corresponding Tracklet object: the exact, approximate, and integer. This was the only choice, since the approximate calculation hadn't been developed yet.

But now that we have the approximate calculation, we can assign the results of that to the approximate parameters and projections. I don't think that needs to be controlled at all by a configuration flag, let alone useapprox, which is already being used to control whether the approximate stub positions are used.

• To clarify my request. People should be able to compare the tracking performance with the exact vs. approx floating point, to ensure that the approx is not doing any harm. I'd assumed that a cfg param in Settings.h must switch between the TTTrack being created from the two options -- so one could then run L1TrackNtuplePlot.C? If not, how is this done?

[aehart]

* To clarify my request. People should be able to compare the tracking performance with the exact vs. approx floating point, to ensure that the approx is not doing any harm. I'd assumed that a cfg param in Settings.h must switch between the TTTrack being created from the two options -- so one could then run L1TrackNtuplePlot.C? If not, how is this done?

@tomalin Right now, I don't think there is an easy way to compare the exact and approximate calculations, either from the TrackletCalculator or TrackletCalculatorDisplaced. Everything downstream of the TrackletCalculatorDisplaced is set up to use the approximate calculation, whether that's the floating point or integer version, since that should match what is ultimately done on the FPGA. For example, when setting the input parameters for the fit:

cmssw/L1Trigger/TrackFindingTracklet/src/HybridFit.cc

Lines 132 to 136 in 3d8ee9b

	double kfrinv = tracklet->rinvapprox();
	double kfphi0 = tracklet->phi0approx();
	double kfz0 = tracklet->z0approx();
	double kft = tracklet->tapprox();
	double kfd0 = tracklet->d0approx();

Probably the most straightforward way of implementing the flag that you're proposing would be to have the methods in the Tracklet class that return the approximate parameters and projections return the exact versions instead, based on the value of the flag. Or the flag could control whether the exact or approximate values are stored in the approximate variables in the constructor. Are there preferences for how this is done?

[tomalin]

* To clarify my request. People should be able to compare the tracking performance with the exact vs. approx floating point, to ensure that the approx is not doing any harm. I'd assumed that a cfg param in Settings.h must switch between the TTTrack being created from the two options -- so one could then run L1TrackNtuplePlot.C? If not, how is this done?

@tomalin Right now, I don't think there is an easy way to compare the exact and approximate calculations, either from the TrackletCalculator or TrackletCalculatorDisplaced. Everything downstream of the TrackletCalculatorDisplaced is set up to use the approximate calculation, whether that's the floating point or integer version, since that should match what is ultimately done on the FPGA. For example, when setting the input parameters for the fit:

cmssw/L1Trigger/TrackFindingTracklet/src/HybridFit.cc

Lines 132 to 136 in 3d8ee9b

	double kfrinv = tracklet->rinvapprox();
	double kfphi0 = tracklet->phi0approx();
	double kfz0 = tracklet->z0approx();
	double kft = tracklet->tapprox();
	double kfd0 = tracklet->d0approx();

Probably the most straightforward way of implementing the flag that you're proposing would be to have the methods in the Tracklet class that return the approximate parameters and projections return the exact versions instead, based on the value of the flag. Or the flag could control whether the exact or approximate values are stored in the approximate variables in the constructor. Are there preferences for how this is done?

• As your answer makes clear that this feature doesn't even exist for the prompt tracking, and would require work to implement, my request is apparently beyond the scope of this PR. So whilst it would be nice to add this option in future, I won't insist on it here.

[tomalin]

I've checked the issues raised during the review have been addressed. Happy to merge.