Implemented NNVtx track-to-vertex association in PUPPI #130
Conversation
There was a problem hiding this comment.
Hello, I commented in a few places where, in my opnion, things need to be changed.
Modifications to the data-format can be minimized. Also, the implementation should aim at having no impact in case the option of not using the NN is set in the configuration.
More in general, what are the plans for a real emulated version of the model instead of the tensorflow implementation?
Also, do you have a pointer to the validation material?
| @@ -22,6 +22,10 @@ namespace l1t { | |||
| int nPar, | |||
| float caloEta, | |||
| float caloPhi, | |||
| ap_ufixed<22, 9> MVA, | |||
There was a problem hiding this comment.
The mva quality is encoded in 3 bits in the track word corresponding to the bins:
{-480, -2.197, -1.386, -0.405, 0.405, 1.386, 2.197, 2.944, 480}
I would suggest adding the a 3 bit ap_uint to the TkObj and proper decoding in:
Conversion to float or ap_ufixed could be implemented via simple lookup where needed.
| @@ -22,6 +22,10 @@ namespace l1t { | |||
| int nPar, | |||
| float caloEta, | |||
| float caloPhi, | |||
| ap_ufixed<22, 9> MVA, | |||
| ap_uint<14> ptEmulationBits, | |||
There was a problem hiding this comment.
Is this used? The same info is already in the TKObj::hwPt
| @@ -22,6 +22,10 @@ namespace l1t { | |||
| int nPar, | |||
| float caloEta, | |||
| float caloPhi, | |||
| ap_ufixed<22, 9> MVA, | |||
| ap_uint<14> ptEmulationBits, | |||
| TTTrack_TrackWord::tanl_t etaEmulationBits, | |||
There was a problem hiding this comment.
Same here. The eta information has already been decoded in the TkObj.
| ap_ufixed<22, 9> MVA, | ||
| ap_uint<14> ptEmulationBits, | ||
| TTTrack_TrackWord::tanl_t etaEmulationBits, | ||
| double Z0, |
There was a problem hiding this comment.
also this one is already decoded in the TkObj
| @@ -228,8 +245,12 @@ namespace l1ct { | |||
| }; | |||
|
|
|||
| struct PVObjEmu : public PVObj { | |||
| double Z0; | |||
There was a problem hiding this comment.
PVObj already decodes the z0 and provide float access.
| @@ -35,6 +36,12 @@ namespace l1ct { | |||
| double priorNe, | |||
| double priorPh, | |||
| pt_t ptCut, | |||
| edm::FileInPath associationGraphPath, | |||
There was a problem hiding this comment.
Same here. I would use a string and create an
edm::FileInPath protected by #ifdef CMSSW_GIT_HASH directive.
| useAssociationNetwork_(useAssociationNetwork), | ||
| associationNetworkZ0binning_(associationNetworkZ0binning), | ||
| associationNetworkEtaBounds_(associationNetworkEtaBounds), | ||
| associationNetworkZ0ResBins_(associationNetworkZ0ResBins), |
There was a problem hiding this comment.
The LinPuppiEmulator should own a NNVtx object rather than its parameter. So that the object could be instantiated only once and used by the LinPuppiEmulator::linpuppi_chs_ref rather than instantiating it for every call.
Moreover, the instantiation of the NNVtx should be sublect to the useAssociationNetwork_ value. Ideally, In CMSSW we would pass directly a PSet to the construtor for flexible configuration.
| edm::FileInPath associationGraphPath_; | ||
| const double associationThreshold_; | ||
| bool useAssociationNetwork_; | ||
| std::vector<double> associationNetworkZ0binning_, associationNetworkEtaBounds_, associationNetworkZ0ResBins_; |
| pf.MVAQualityBits = track.MVAQualityBits; | ||
| pf.ptEmulationBits = track.ptEmulationBits; | ||
| pf.etaEmulationBits = track.etaEmulationBits; | ||
| pf.Z0 = track.Z0; |
There was a problem hiding this comment.
I don't see the corresponding changes to the PFChargedObjEmu in the diff. Also in this case, however, I assueme the only needed addition are the 3 bits of the MVA bins.
| associationNetworkZ0binning_, | ||
| associationNetworkEtaBounds_, | ||
| associationNetworkZ0ResBins_); | ||
|
|
There was a problem hiding this comment.
What are the plans to move to an hls4ml emulation?
Can the tensorflow calls be encapsulated into the NNVtx constructor (assuming an hls4ml emulation is on its way and will replace the tensorflow calls).
Even more important, here we recreate the NNVtx object every time the linpuppi_chs_ref is called. This is once per region per every event.
Also, if useAssociationNetwork_==false we can avoid evaluating the network alltogether, isn't it?
PR description:
The NNVtx track-to-vertex association is implemented within PUPPI. A switch for the association network is added to the L1 correlator layer 1 configuration. The association network in CMS data is used.
PR validation:
The PR has been validated by running L1 nano for the VBF Higs to tau tau sample and checking the matching efficiency of NN Tau, where there is an improvement of ~10-20% in the efficiency over the baseline FastHisto algorithm.