fix matchtp variables #76

cgsavard · 2021-03-09T03:13:23Z

PR description:
The trk_matchtp_{} variables in the standard ntuple maker had some issues. First, the trk_matchtp_z0 was incorrect. It was actually the vz, not z0. Next, trk_matchtp_d0 was not included in the collection but is an important variable to have for studying displaced tracks. Lastly, many of the calls to get the variables, such as the tp phi, were outdated although it still worked.

I made all these fixes by followiing exactly what was done in the regular tp collection to get/calculate all of the variables.

PR validation:
Here are the distributions of the trk_matchtp_z0 and trk_matchtp_d0 to show that they are calculated correctly. The rest of the matchtp variables did not change.

skinnari · 2021-03-09T15:47:24Z

hi @cgsavard - thanks for this cleanup. i am going to hold off on this a few days since we are trying to merge code from Anders to avoid further confusion with that PR.

tomalin · 2021-03-09T17:56:09Z

I thought we'd fixed the d0 & z0 and their signs two years ago. But they still look weird.

The eqn. for trk_d0 https://github.com/cgsavard/cmssw/blob/fix_matchtp/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L868 has wrong sign compared with the usual CMS definition of d0 https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrackReco/interface/TrackBase.h#L611 .
If I plot tp_d0_prod vs tp_d0 , I see they are anti-correlated, so one of them is wrong. It is the sign of tp_d0_prod which looks correct compared with the usual CMS definition, and the sign of tp_d0 that looks wrong.
matchtrk_d0 also appears to have wrong sign too.

tomalin · 2021-03-09T17:58:52Z

L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc

+
+	float tmp_matchtp_charge = my_tp->charge();
+	float K = Kmagnitude * tmp_matchtp_charge;
+	float d = 0;


What is the point of "float d = 0" if its value is always zero?

Good question, I'm not sure. I just followed exactly what was done in the tracking particle section of the code and this variable was there but I did not write it. I can take it out of both sections if you think it's not important?

@skinnari any objections if Claire removes "float d"?

As far as I can see, this "K" is inv(2R). I don't know either what is purpose of "d".

I have removed the d=0 variable both here and in calculating the tp_d0

cgsavard · 2021-03-09T18:19:19Z

The eqn. for trk_d0 https://github.com/cgsavard/cmssw/blob/fix_matchtp/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L868 has wrong sign compared with the usual CMS definition of d0 https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrackReco/interface/TrackBase.h#L611 .

If I plot tp_d0_prod vs tp_d0 , I see they are anti-correlated, so one of them is wrong. It is the sign of tp_d0_prod which looks correct compared with the usual CMS definition, and the sign of tp_d0 that looks wrong.

matchtrk_d0 also appears to have wrong sign too.

I did not look into any other d0 variables, I just took what was already there to be the truth and copied it to create trk_matchtp_d0. If you would like to make these fixes in this PR then let me know and I can commit some more changes.

Jingyan95 · 2021-04-09T08:59:44Z

L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc

+	float delx = -tmp_matchtp_vx;
+	float dely = -tmp_matchtp_vy;
+
+	float A = 0.01 * 0.5696;


This float "A" basically contains information of B filed that needs to be re
placed with a constant.

I changed this to now depend on the magnetic field and speed of light constants in cmssw. I also redefined the K variable to r2_inv to avoid confusion with this variable in the future. I double checked to make sure that the K and r2_inv matched after the switch to using the cms constants.

Jingyan95 · 2021-04-09T12:45:24Z

L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc

+
+	float tmp_matchtp_t = tan(2.0 * atan(1.0) - 2.0 * atan(exp(-tmp_matchtp_eta)));
+
+	float delx = -tmp_matchtp_vx;


I think the minus signs on L1021 and L1022 might be wrong.

I don't know enough about propagating back to know if this is wrong or not. If you can confirm that this is incorrect then I will make the change. Do you also think that the overall sign of d0 is wrong? Ian proposes that it might be incorrect.

To your second point on whether or not the overall sign of d0 is wrong, I agree with Ian that the sign of d0 does not seem to follow the cms convention. The easiest way to see this is to plot tp_d0 vs tp_d0_prod

There seems to be a negative association between the two variables and I am pretty sure the sign of tp_d0_prod is correct.
The only way I can think of to fix this sign problem is NOT directly flipping the sign on L1163 (i.e. tmp_tp_d0). Instead, I believe the signs on L1151&L1152 (delx & dely) are incorrect. If we modify those two lines and plot tp_d0 vs tp_d0_prod again:
.
Let me talk to Louise to see we can reach an agreement.

tomalin · 2021-04-28T06:59:39Z

Can we please finish this PR, before we merge cms-L1TK into official CMSSW (~1 week)? I think we just need to fix the d0 signs.

Does everyone agree that the signs of tmp_trk_d0 (L864) and tmp_matchtrk_d0 (L1352) need multiplying by -1, to be consistent with the usual CMS convention? (This would also make the expression used to derive it look consistent with that for tmp_tp_d0_prod at L1093).
Like Jack, I believe that the sign of tmp_tp_d0_prod is correct.
Jack's plot proves there is also a problem with the sign of tmp_tp_d0. I have not checked his statement that to fix this, one should not reverse the sign of tmp_tp_d0, but instead that of delx & dely (L1151). Can someone please do so?
Jack's plot also shows that tmp_tp_d0 is often zero, even with tmp_tp_d0_prod is not. I assume this is because of the cuts applied at L1095-1105. Is there a good reason why we don't bother calculating tmp_tp_d0 if its Pt < 2 GeV etc.?

Jingyan95 · 2021-04-30T08:38:09Z

Can we please finish this PR, before we merge cms-L1TK into official CMSSW (~1 week)? I think we just need to fix the d0 signs.

Does everyone agree that the signs of tmp_trk_d0 (L864) and tmp_matchtrk_d0 (L1352) need multiplying by -1, to be consistent with the usual CMS convention? (This would also make the expression used to derive it look consistent with that for tmp_tp_d0_prod at L1093).

Like Jack, I believe that the sign of tmp_tp_d0_prod is correct.

Jack's plot proves there is also a problem with the sign of tmp_tp_d0. I have not checked his statement that to fix this, one should not reverse the sign of tmp_tp_d0, but instead that of delx & dely (L1151). Can someone please do so?

Jack's plot also shows that tmp_tp_d0 is often zero, even with tmp_tp_d0_prod is not. I assume this is because of the cuts applied at L1095-1105. Is there a good reason why we don't bother calculating tmp_tp_d0 if its Pt < 2 GeV etc.?

Hi Ian,

-1 I completely agree that the sign of tmp_trk_d0 and tmp_matchtrk_d0 need multiplying by -1.
-3. The fix should be either reversing the signs of delx & dely or that of tmp_tp_d0 depending on the convention of curvature. After studying this a bit further, I would like to retract my formal statement and just say reversing the sign of tmp_tp_d0 is the right way to go. I had some discussion with @skinnari about this but haven't reached an agreement with her.
-4. I am not entirely sure about the 2GeV cut. It does feel a bit odd to me though.

If finishing this PR before merging into official CMSSW is the goal, I would recommend the following ways forward regarding the 3rd bullet point:
We find a volunteer to check whether or not reversing the sign of tmp_tp_d0 is the correct.
Or we bring this up during the meeting next Friday. I am happy to prepare a couple of slides if needed.

tomalin · 2021-04-30T14:17:26Z

@cgsavard based on discussion above, please:
a) Reverse sign of sign of tmp_trk_d0 and tmp_matchtrk_d0.
b) Move the calculation of tmp_tp_d0 and tmp_tp_z0 up from its current location https://github.com/cgsavard/cmssw/blob/fix_matchtp/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L1146 to new location https://github.com/cgsavard/cmssw/blob/fix_matchtp/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L1132 , so they are evaluated whenever the "prod" variables are.
c) Looking at the maths above https://github.com/cgsavard/cmssw/blob/fix_matchtp/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L1163 , my suspicion is that reversing the sign of (delx,dely) will give a negligibly different result to reversing the sign of tmp_tp_d0. I suggest you do the calc both ways, and add debug printout of the difference in the two results to check this. Run on displaced MC with low Pt particles, where the effect should be biggest. If both methods give almost identical results, then you can just reverse the sign of tmp_t0_d0.

cgsavard · 2021-04-30T22:48:00Z

I made the first two changes.

c) Looking at the maths above https://github.com/cgsavard/cmssw/blob/fix_matchtp/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L1163 , my suspicion is that reversing the sign of (delx,dely) will give a negligibly different result to reversing the sign of tmp_tp_d0. I suggest you do the calc both ways, and add debug printout of the difference in the two results to check this. Run on displaced MC with low Pt particles, where the effect should be biggest. If both methods give almost identical results, then you can just reverse the sign of tmp_t0_d0.

When looking at the difference in a ttbar+200PU sample, most changes are negligible but there are a few on the order of 10^-1 and 10^-2 which seem like a big enough change to look further. I could not run the displaced particle sample (https://twiki.cern.ch/twiki/bin/view/CMS/L1TrackMC#CMSSW_11_3_0) as the files failed to open and there doesn't seem to be another displaced dataset currently ready to use.

tomalin · 2021-05-01T11:06:29Z

@cgsavard try running on the displaced MC https://cmsweb.cern.ch/das/request?view=list&limit=50&instance=prod%2Fglobal&input=%2FRelVal*Displaced*%2FCMSSW_11_3_0_pre6*%2FGEN-SIM-DIGI-RAW . (N.B. It's D76, so you'll have to change the geometry in cfg). Checking which eqn. gives best d0 resolution might also help.

cgsavard · 2021-05-03T18:16:03Z

@tomalin The results are very similar. Here I have attached the results for the displaced SUSY sample (noPU). "tp_d0" is the current d0 calculation with a sign flip at the end, and "tp_d02" is the d0 calculation when delx and dely have a sign flip. We see in both this and with the ttbar sample that the resolution for "tp_d02" is slightly larger than for "tp_d0". For comparison, the std of the difference tp_d0-tp_d02 for the ttbar sample is 0.4442 instead of 0.8873 for the displaced SUSY sample shown here.

Since the results are very similar and the difference is small, should we select tp_d0 with the slightly smaller resolution?

skinnari · 2021-05-04T15:58:47Z

hi all, I am trying to understand this thread and am a bit confused. AFAIK the known problem is with the sign of track d0 vs TP d0? are you saying there is another problem here?

tomalin · 2021-05-04T19:58:02Z

hi all, I am trying to understand this thread and am a bit confused. AFAIK the known problem is with the sign of track d0 vs TP d0? are you saying there is another problem here?

Hi Louise, the signs of tmp_trk_d0 and tmp_matchtrk_d0 were wrong, which this PR has now fixed.
The sign of tmp_tp_d0 is also approximately wrong, but we're unsure if it should simply be multiplied by -1, or if instead the signs of the variables delx & dely used to calculate it should be multiplied by -1. Both alternatives give almost identical results.

skinnari · 2021-05-07T16:32:21Z

@cgsavard @Jingyan95 I am confused now when looking at the changes. I know there is a problem with the d0 sign, but I thought it was only the (-1) that needed to be removed for the TP. but it seems that you are also changing the sign of the track d0?

Jingyan95 · 2021-05-07T17:20:58Z

@cgsavard @Jingyan95 I am confused now when looking at the changes. I know there is a problem with the d0 sign, but I thought it was only the (-1) that needed to be removed for the TP. but it seems that you are also changing the sign of the track d0?

That's right. The signs of tmp_trk_d0 [1] and tmp_matchtrk_d0 [2] are also incorrect. Currently these two take the following form:
-Vx * sin(phi)+ Vy * cos(phi)
We think the correct form should be
Vx * sin(phi)- Vy * cos(phi)
They are off by -1. An example of the correct form can be found here [3]. An even better example would be [4]

[1] https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-11_3_0_pre3/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L864
[2] https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-11_3_0_pre3/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L1352
[3] https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-11_3_0_pre3/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker.cc#L1093
[4] https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrackReco/interface/TrackBase.h#L611]

cgsavard · 2021-05-12T17:26:48Z

@tomalin I removed the sign flip on tp_d0. I believe that was the last thing we were waiting on, unless you can think of anything else?

tomalin · 2021-05-17T16:04:28Z

As a sanity check, I've verified that with the latest version of this branch, 2D plots of matchtrk_d0 vs. tp_d0 or tp_d0_prod show a line gradient of +1. (And the resolution is much better for tp_d0 than for tp_d0_prod, as one would expect).

tomalin

Happy

* fix matchtp variables * remove d=0 variables * switch values for cms constants * change pi to constant * change trk_d0 sign * move tmp_tp_d0 and tmp_tp_z0 calc up * flip tp_d0 sign

fix matchtp variables

89de641

cgsavard requested a review from skinnari March 9, 2021 03:13

tomalin self-requested a review March 9, 2021 16:15

tomalin reviewed Mar 9, 2021

View reviewed changes

tomalin requested a review from Jingyan95 April 7, 2021 15:50

Jingyan95 reviewed Apr 9, 2021

View reviewed changes

cgsavard added 3 commits April 13, 2021 12:02

remove d=0 variables

9c5fcc6

switch values for cms constants

a66c6bd

change pi to constant

3f2503d

cgsavard added 2 commits April 30, 2021 16:34

change trk_d0 sign

538d432

move tmp_tp_d0 and tmp_tp_z0 calc up

6aae580

flip tp_d0 sign

56385d1

tomalin approved these changes May 17, 2021

View reviewed changes

tomalin merged commit 3489b50 into cms-L1TK:L1TK-dev-11_3_0_pre3 May 17, 2021

skinnari pushed a commit that referenced this pull request May 20, 2021

fix matchtp variables (#76)

6363c2d

* fix matchtp variables * remove d=0 variables * switch values for cms constants * change pi to constant * change trk_d0 sign * move tmp_tp_d0 and tmp_tp_z0 calc up * flip tp_d0 sign

skinnari pushed a commit that referenced this pull request Jun 8, 2021

fix matchtp variables (#76)

bcb450b

* fix matchtp variables * remove d=0 variables * switch values for cms constants * change pi to constant * change trk_d0 sign * move tmp_tp_d0 and tmp_tp_z0 calc up * flip tp_d0 sign

skinnari pushed a commit that referenced this pull request Jun 22, 2021

fix matchtp variables (#76)

69c4bce

* fix matchtp variables * remove d=0 variables * switch values for cms constants * change pi to constant * change trk_d0 sign * move tmp_tp_d0 and tmp_tp_z0 calc up * flip tp_d0 sign

skinnari pushed a commit that referenced this pull request Jun 22, 2021

fix matchtp variables (#76)

4722e72

* fix matchtp variables * remove d=0 variables * switch values for cms constants * change pi to constant * change trk_d0 sign * move tmp_tp_d0 and tmp_tp_z0 calc up * flip tp_d0 sign

skinnari pushed a commit that referenced this pull request Jul 9, 2021

fix matchtp variables (#76)

e2aca4d

* fix matchtp variables * remove d=0 variables * switch values for cms constants * change pi to constant * change trk_d0 sign * move tmp_tp_d0 and tmp_tp_z0 calc up * flip tp_d0 sign

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix matchtp variables #76

fix matchtp variables #76

cgsavard commented Mar 9, 2021

skinnari commented Mar 9, 2021

tomalin commented Mar 9, 2021

tomalin Mar 9, 2021

cgsavard Mar 9, 2021

tomalin Apr 8, 2021

Jingyan95 Apr 9, 2021

cgsavard Apr 13, 2021

cgsavard commented Mar 9, 2021

Jingyan95 Apr 9, 2021

cgsavard Apr 13, 2021

Jingyan95 Apr 9, 2021

cgsavard Apr 13, 2021

Jingyan95 Apr 14, 2021 •

edited by tomalin

tomalin commented Apr 28, 2021 •

edited

Jingyan95 commented Apr 30, 2021 •

edited

tomalin commented Apr 30, 2021

cgsavard commented Apr 30, 2021

tomalin commented May 1, 2021

cgsavard commented May 3, 2021

skinnari commented May 4, 2021

tomalin commented May 4, 2021

skinnari commented May 7, 2021

Jingyan95 commented May 7, 2021 •

edited

cgsavard commented May 12, 2021

tomalin commented May 17, 2021

tomalin left a comment


		float tmp_matchtp_t = tan(2.0 * atan(1.0) - 2.0 * atan(exp(-tmp_matchtp_eta)));

		float delx = -tmp_matchtp_vx;

fix matchtp variables #76

fix matchtp variables #76

Conversation

cgsavard commented Mar 9, 2021

skinnari commented Mar 9, 2021

tomalin commented Mar 9, 2021

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

cgsavard commented Mar 9, 2021

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

Jingyan95 Apr 14, 2021 • edited by tomalin

Choose a reason for hiding this comment

tomalin commented Apr 28, 2021 • edited

Jingyan95 commented Apr 30, 2021 • edited

tomalin commented Apr 30, 2021

cgsavard commented Apr 30, 2021

tomalin commented May 1, 2021

cgsavard commented May 3, 2021

skinnari commented May 4, 2021

tomalin commented May 4, 2021

skinnari commented May 7, 2021

Jingyan95 commented May 7, 2021 • edited

cgsavard commented May 12, 2021

tomalin commented May 17, 2021

tomalin left a comment

Choose a reason for hiding this comment

Jingyan95 Apr 14, 2021 •

edited by tomalin

tomalin commented Apr 28, 2021 •

edited

Jingyan95 commented Apr 30, 2021 •

edited

Jingyan95 commented May 7, 2021 •

edited