search

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

Update track quality chi2 bins #161

Closed cgsavard closed 2 years ago

cgsavard commented 2 years ago

PR description:

This PR updates the chi2rz, chi2rphi, and bendchi2 binnings used by the track quality classifier. These binning were proposed by Emily MacDonald and are the current accepted bins used by the L1TK group, and I have previously shown here that updating these bins in the track quality classifier does not effect the classifier performance. This PR is correlated to a PR made to cms-data to update the default classifier to one trained on the new binning.

PR validation:

I have checked that the python implementation and CMSSW implementation of the new classifier with these chi2 bin changes have matching output. I have also verified that the overall performance of this new classifier almost exactly matches the performance of the old classifier in CMSSW, just like what I showed in the python implementations here.

tomalin commented 2 years ago

TrackQuality gets some cfg params such as "maxZ0" from python cfg. So why does it hard-wire the chi2 binning?

Though I assume the chi2 bins here are the same as the ones in https://github.com/cms-sw/cmssw/blob/5bc18d0ced1478749b1e9364a351734e3b973f01/DataFormats/L1TrackTrigger/interface/TTTrack_TrackWord.h#L100 , where they are also hard-wired.

It's horrible to have the same hard-wired numbers in two different places, and need to keep them in phase. Perhaps they should be moved to a header file, containing only these constants, which both TTTrack_TrackWord.h and TrackQuality can include? (In which case it would have to go in DataFormats/L1TrackTrigger/ since DataFormats/ can't depend on L1Trigger/.

Not sure.

tomalin commented 2 years ago

Since your PR to cms-data has not yet been merged, the git CI running here is using the old cms-data files. But it's tracking efficiency looks unchanged. Is this a surprise?

cgsavard commented 2 years ago

I agree that it would be nice to have the bins hard-wired elsewhere and we can just pull from there. I don't know where is best but am open to any suggestions.

What is the tracking efficiency a measure of? Because I don't think track quality is actually currently being used in that definition so a change in track quality shouldn't change that. Track quality is only being calculated and put in the ntuple at the moment.

tomalin commented 2 years ago

Why is TrackQuality digitizing the chi2 itself at https://github.com/cms-L1TK/cmssw/blob/update_tq/L1Trigger/TrackTrigger/src/TrackQuality.cc#L127 , rather than calling aTrack.get_chi2XYBits() ? Wouldn't this eliminate the need to hard-wire the chi2 bins inside TrackQuality?

cgsavard commented 2 years ago

Why is TrackQuality digitizing the chi2 itself at https://github.com/cms-L1TK/cmssw/blob/update_tq/L1Trigger/TrackTrigger/src/TrackQuality.cc#L127 , rather than calling aTrack.get_chi2XYBits() ? Wouldn't this eliminate the need to hard-wire the chi2 bins inside TrackQuality?

This is a great idea. There are two issues I see at the moment. First off, the bins defined in the TTTrack word are not the newest binning https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_0_0_pre4/DataFormats/L1TrackTrigger/src/TTTrack_TrackWord.cc#L383-L421. Should I update these bins as well or is there a reason why these are still the old bins? This would also require updating the TTTrack word to pass through the chi2 variables pdof. The other issue is that there is not a function to get the chi2rz bins, but that can easily be created as well.

tomalin commented 2 years ago

Why is TrackQuality digitizing the chi2 itself at https://github.com/cms-L1TK/cmssw/blob/update_tq/L1Trigger/TrackTrigger/src/TrackQuality.cc#L127 , rather than calling aTrack.get_chi2XYBits() ? Wouldn't this eliminate the need to hard-wire the chi2 bins inside TrackQuality?

This is a great idea. There are two issues I see at the moment. First off, the bins defined in the TTTrack word are not the newest binning https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_0_0_pre4/DataFormats/L1TrackTrigger/src/TTTrack_TrackWord.cc#L383-L421. Should I update these bins as well or is there a reason why these are still the old bins? This would also require updating the TTTrack word to pass through the chi2 variables pdof. The other issue is that there is not a function to get the chi2rz bins, but that can easily be created as well.

1) You do not need to update the chi2 binning in TTTrack_TrackWord, since that update is already done in the official CMSSW release. https://github.com/cms-sw/cmssw/blob/master/DataFormats/L1TrackTrigger/interface/TTTrack_TrackWord.h#L99 . I think the digitized chi2rphi value is available in the CMSSW release, perhaps at https://github.com/cms-sw/cmssw/blob/master/DataFormats/L1TrackTrigger/interface/TTTrack_TrackWord.h#L207 (I've not checked). If so, then we should add my suggestion after I've rebased cms-L1TK to latest CMSSW release.