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

Fill MVA1 when using newKF #224

Closed cgsavard closed 1 year ago

cgsavard commented 1 year ago

PR description:

When using the new KF, TTTracks are converted from the KFTracks here. The trk_MVA1 variable is set to 0 however, and so this PR runs the track quality code to fill this variable.

PR validation:

The L1TrackNtupleMaker ntuple is now filled with non-zero trk_MVA1 variables.

cgsavard commented 1 year ago

I'm not sure why the check failed; can someone advise me on how to fix this?

tschuh commented 1 year ago

I don't know why it failed and have no clue how to prompt retest apart from making a dummy commit (e.g. adding a comment somewhere).

tschuh commented 1 year ago

Could you please make L1TrackQuality as an ESProduct available?

cgsavard commented 1 year ago

Could you please make L1TrackQuality as an ESProduct available?

What do you mean by this? I'm not clear on these instructions

tschuh commented 1 year ago

Instead of having each user of L1TrackQuality constructing this class (potential with a different configuration by mistake) one could construct L1TrackQuality once and put it into the Event Setup from where each user can access and use it. That is the official cmssw way of sharing algorithms. Here is an twiki article: https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToWriteESProducer and here is an example from our code: https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_6_0_pre5/L1Trigger/TrackFindingTracklet/plugins/ProducerChannelAssignment.cc

cgsavard commented 1 year ago

@tschuh I see. When the track quality code was first set up, there was a discussion on how to integrate it in the current code. @Chriisbrown might be able to say more about why it was set up in this manner instead of how you are suggesting.

However, this topic is not something for this PR to fix. If we want to change the code structure then that should be a new PR. This PR is just making sure that we have the current code running correctly so that people right now can use it with the newKF turned on.

cgsavard commented 1 year ago

Closing in favor of #228 which makes the PR from an L1Tk branch instead of my personal fork for the CI checks