cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

fix matchtp variables #76

Closed cgsavard closed 3 years ago

cgsavard commented 3 years ago

PR description: The trkmatchtp{} 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.

Screen Shot 2021-03-08 at 7 56 33 PM Screen Shot 2021-03-08 at 7 58 49 PM
skinnari commented 3 years ago

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 commented 3 years ago

I thought we'd fixed the d0 & z0 and their signs two years ago. But they still look weird. 1) 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 . 2) 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. 3) matchtrk_d0 also appears to have wrong sign too.

cgsavard commented 3 years ago
  1. 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 .
  2. 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.
  3. 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.

tomalin commented 3 years ago

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. 1) 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). 2) Like Jack, I believe that the sign of tmp_tp_d0_prod is correct. 3) 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? 4) 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 commented 3 years ago

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.

  1. 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).
  2. Like Jack, I believe that the sign of tmp_tp_d0_prod is correct.
  3. 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?
  4. 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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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?

dispsusy_tp_d0 dispsusy_tp_d02 dispsusy_tp_diff

skinnari commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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).