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

Change binning method used by track quality #163

Closed cgsavard closed 2 years ago

cgsavard commented 2 years ago

PR description:

This PR updates the binning method in the track quality class with the chi2 variables. These variables are now taken directly from the TTTrack word.

PR validation:

I have tested to make sure that the old binning method produced the exact same results as the bins pulled directly from the TTTrack word. Therefore, this does not affect the performance of the classifier at all.

tomalin commented 2 years ago

@cgsavard The reason git CI failed is that you made a PR from your personal cgsavard fork, instead of from cms-L1TK. You need to use the latter, so the git has the privileges it needs to run CI.

cgsavard commented 2 years ago

@tomalin Does this mean that I will have to redo the PR from a different branch?

Yes, I have checked that the functions are returning the proper bins.

tomalin commented 2 years ago

@tomalin Does this mean that I will have to redo the PR from a different branch?

Yes, I have checked that the functions are returning the proper bins.

i'm afraid so, yes.

tomalin commented 2 years ago

Magic numbers in the code, like "6" here are not allowed https://github.com/cgsavard/cmssw/blob/update_chi2_tq_bins/L1Trigger/TrackTrigger/src/TrackQuality.cc#L116 . Use named constants.

tomalin commented 2 years ago

Add a comment to https://github.com/cgsavard/cmssw/blob/update_chi2_tq_bins/L1Trigger/TrackTrigger/src/TrackQuality.cc#L54 explaining that the eta_bins should be identical to those in https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_2_4/L1Trigger/TrackFindingTMTT/src/Settings.cc#L27 . I wish we could eliminate this duplication ...

tomalin commented 2 years ago

@Jingyan95 do you know where the table of constants "hitmap" near https://github.com/cgsavard/cmssw/blob/update_chi2_tq_bins/L1Trigger/TrackTrigger/src/TrackQuality.cc#L59 come from. I thought they must correspond to constants in the KF, but I can't see them there. We should add a comment to code explaining how they're derived.

cgsavard commented 2 years ago

@tomalin A lot of these variables that are being calculated are not used and so we can actually just remove most of these variables. We included them previously with the thought that someone could test their own track quality classifier with different variables, but I now am in favor of removing all of the extra non-used code and letting the user create their own variables if they wish to test a new classifier. Do you agree with this @Chriisbrown ?

Chriisbrown commented 2 years ago

@tomalin A lot of these variables that are being calculated are not used and so we can actually just remove most of these variables. We included them previously with the thought that someone could test their own track quality classifier with different variables, but I now am in favor of removing all of the extra non-used code and letting the user create their own variables if they wish to test a new classifier. Do you agree with this @Chriisbrown ?

I am happy for us to streamline the code and just leave the variables we use for the BDTs we have saved in cms-data. Maybe add a comment at the top of the file describing how to create new variables for the ONNX models if a new model was developed.

cgsavard commented 2 years ago

This PR has been superceded by #165 therefore this one will be closed.