GBDT-Based Displaced Vertex Producer #1257

ryanm124 · 2024-05-14T08:58:32Z

Updated version of the displaced vertex producer presented at L1 DPG: https://indico.cern.ch/event/1408668/contributions/5920520/attachments/2850847/4984793/Displaced%20Vertexing%20Low%20Fake%20DP%20Note.pdf

Contains the latest GBDT trained on Dark Photon and Neutrino Gun sample emulated tracks.

PR for displaced vertex GBDT: cms-data/L1Trigger-L1TTrackMatch#2

…e, and updating displaced vertex BDT

triggerDoctor · 2024-05-14T12:43:35Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-05-14T13:04:46Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found issues with the headers. These issues likely arise from improper build files.

The following is the stderr of the header checking:



In file included from src/L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h:12:
/cvmfs/cms.cern.ch/el9_amd64_gcc12/cms/cmssw/CMSSW_14_0_0_pre3/src/PhysicsTools/ONNXRuntime/interface/ONNXRuntime.h:19:10: fatal error: onnxruntime/core/session/onnxruntime_cxx_api.h: No such file or directory
   19 | #include "onnxruntime/core/session/onnxruntime_cxx_api.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
gmake: *** [config/SCRAM/GMake/Makefile.rules:1833: tmp/el9_amd64_gcc12/src/L1Trigger/L1TTrackMatch/src/L1TriggerL1TTrackMatch/DisplacedVertexProducer.h.chk_header] Error 1
gmake: Target 'check-headers' not remade because of errors.

Info	Value
return code	`2`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-05-14T13:05:22Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 10 files that did not meet formatting requirements:

DataFormats/L1Trigger/interface/DisplacedVertex.h
L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc
L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h
L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc
L1Trigger/L1TTrackMatch/test/Analyzer_DisplacedMuon.C
L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc
L1Trigger/Phase2L1GMT/interface/TPSLUTs.h
L1Trigger/Phase2L1GMT/plugins/Phase2L1TGMTFwdMuonTranslator.cc
L1Trigger/TrackFindingTracklet/plugins/L1FPGATrackProducer.cc
L1Trigger/TrackTrigger/src/L1TrackQuality.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

triggerDoctor · 2024-05-14T13:26:55Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

This PR passes available unit tests!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b runtests`

triggerDoctor · 2024-05-14T13:37:13Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation

I found a non-zero return code running the relval workflows for this PR!

Info	Value
return code	`1`
command	`eval` scramv1 runtime -sh `&& runTheMatrix.py --what upgrade -l 26834.78`

cgsavard · 2024-05-14T13:53:22Z

L1Trigger/TrackTrigger/data/clf_GBDT_newbin_disp_xgb.onnx

We normally put these files here (https://github.com/cms-data/L1Trigger-TrackTrigger) to avoid having non-readable files in this repository. You will have to put in a PR to that repository to put it in there.

Done, cms-data/L1Trigger-TrackTrigger#4

cgsavard · 2024-05-14T13:54:47Z

L1Trigger/TrackTrigger/src/L1TrackQuality.cc

@@ -90,7 +90,7 @@ std::vector<float> L1TrackQuality::featureTransform(TTTrack<Ref_Phase2TrackerDig
  feature_map["bendchi2_bin"] = tmp_trk_bendchi2_bin;
  feature_map["chi2rphi_bin"] = tmp_trk_chi2rphi_bin;
  feature_map["chi2rz_bin"] = tmp_trk_chi2rz_bin;
-  feature_map["tanl"] = tmp_trk_tanl;


Why is this being removed? Doesn't the prompt classifier need this?

Just added this back in commit dbf8525

cgsavard · 2024-05-14T14:04:20Z

L1Trigger/TrackTrigger/src/L1TrackQuality.cc

@@ -159,7 +159,9 @@ void L1TrackQuality::setL1TrackQuality(TTTrack<Ref_Phase2TrackerDigi_>& aTrack)
    }

    else if (this->qualityAlgorithm_ == QualityAlgorithm::GBDT) {
-      aTrack.settrkMVA1(ortoutputs[1][1]);
+      if (this->featureNames_.back() == "d0"){ // temp fix, need better way to fill disp MVA


This really should have a better solution. I would suggest passing the output value through the function and setting the MVA variable in L1FPGATrackProducer.cc with an extended tracking flag since we shouldn't run this BDT unless extended tracking is running because d0 isn't filled.

I've implemented your suggestion in commit dbf8525

cgsavard · 2024-05-14T14:05:56Z

L1Trigger/TrackFindingTracklet/plugins/L1FPGATrackProducer.cc

    if (trackQuality_) {
      trackQualityModel_->setL1TrackQuality(aTrack);
+      trackQualityDispModel_->setL1TrackQuality(aTrack);


There should be an extended tracking flag here since this should only run for extended tracking. There is a extended_ variable in this file that can be used.

Done here dbf8525

cgsavard · 2024-05-14T14:10:27Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker_cfg.py

@@ -42,27 +42,28 @@
 # input and output
 ############################################################

-process.maxEvents = cms.untracked.PSet(input = cms.untracked.int32(20))
+


All the configurations in this file should be set back to the default configurations used by everyone. So the maxEvents, readFiles, skipEvents, useJobReport, fileName, etc.

Reverted these changes as part of d95ffe9

This also goes for the readFiles

Chriisbrown · 2024-05-15T19:15:59Z

L1Trigger/L1TTrackMatch/python/DisplacedVertexProducer_cfi.py

+  l1TracksInputTag = cms.InputTag("l1tTTTracksFromExtendedTrackletEmulation", "Level1TTTracks"),
+  l1TrackVertexCollectionName = cms.string("dispVertices"),
+  mcTruthTrackInputTag = cms.InputTag("TTTrackAssociatorFromPixelDigisExtended", "Level1TTTracks"),
+  ONNXmodel = cms.string("/afs/cern.ch/user/r/rmccarth/private/dispVert/CMSSW_14_0_0_pre3/src/L1Trigger/L1TTrackMatch/test/trackAndVert_NewTQMVA_model.onnx"),


Similar to Claire's comment below this also needs to be in a cms-data repository, we have one for L1TTrackMatch here: https://github.com/cms-data/L1Trigger-L1TTrackMatch

Just made a PR: cms-data/L1Trigger-L1TTrackMatch#2

Chriisbrown · 2024-05-15T19:18:22Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+      if(fabs(vertex.b.d0)<fabs(minD0)) minD0 = vertex.b.d0;
+      //std::vector<float> Transformed_features = {vertex.delta_z, vertex.R_T, vertex.cos_T, vertex.d_T, vertex.chi2rzdofSum, float(vertex.numStubsSum), vertex.chi2rphidofSum, minD0, vertex.a.pt+vertex.b.pt};
+      std::vector<float> Transformed_features = {selectedTracks[i].pt, selectedTracks[j].pt, selectedTracks[i].eta, selectedTracks[j].eta, selectedTracks[i].phi, selectedTracks[j].phi, selectedTracks[i].d0, selectedTracks[j].d0, selectedTracks[i].z0, selectedTracks[j].z0, selectedTracks[i].chi2rz, selectedTracks[j].chi2rz, selectedTracks[i].bendchi2, selectedTracks[j].bendchi2, selectedTracks[i].MVA1, selectedTracks[j].MVA1, selectedTracks[i].MVA2, selectedTracks[j].MVA2, vertex.d_T, vertex.R_T, vertex.cos_T, vertex.delta_z};
+      //cms::Ort::ONNXRuntime Runtime(this->ONNXmodel_);  //Setup ONNX runtime


This comment doesn't need to be here, in fact we should remove the similar comment from the TrackQuality.cc as the Runtime is now initialised (as you do) in the constructor

Removed in commit 63cdfcf

Chriisbrown · 2024-05-15T19:19:58Z

L1Trigger/L1TTrackMatch/python/DisplacedVertexProducer_cfi.py

+  mcTruthTrackInputTag = cms.InputTag("TTTrackAssociatorFromPixelDigisExtended", "Level1TTTracks"),
+  ONNXmodel = cms.string("/afs/cern.ch/user/r/rmccarth/private/dispVert/CMSSW_14_0_0_pre3/src/L1Trigger/L1TTrackMatch/test/trackAndVert_NewTQMVA_model.onnx"),
+  ONNXInputName = cms.string("feature_input"),
+  featureNames = cms.vstring(['trkExt_pt_firstTrk', 'trkExt_pt', 'trkExt_eta_firstTrk', 'trkExt_eta', 'trkExt_phi_firstTrk', 'trkExt_phi', 'trkExt_d0_firstTrk', 'trkExt_d0', 'trkExt_z0_firstTrk', 'trkExt_z0', 'trkExt_chi2rz_firstTrk', 'trkExt_chi2rz', 'trkExt_bendchi2_firstTrk', 'trkExt_bendchi2', 'trkExt_MVA_firstTrk', 'trkExt_MVA', 'trkExt_MVA2_firstTrk', 'trkExt_MVA2', 'dv_d_T', 'dv_R_T', 'dv_cos_T', 'dv_del_Z'])


Are these featureNames used by the code anywhere, in the track quality code they are used in a mapping but you just use all of these in a single vector

They are read in by the DisplacedVertexProducer (https://github.com/ryanm124/cmssw/blob/d95ffe97501c7e003b3bc9de7e83685ecc20a6a6/L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc#L112) but not actually used. I could build up a map of inputs using this list or just remove the list entirely, whichever you think is best.

We have a map of inputs in the track quality to allow some flexibility over the inputs but if your inputs are set and you don't foresee any change I would not have these features names read in and unused and instead have a comment here in the python config about what features the BDT uses

I've turned this into a comment as you suggested here 63cdfcf

Chriisbrown · 2024-05-15T19:47:28Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+    float MVA2 = iterL1Track->trkMVA2();
+    std::vector<edm::Ref<edmNew::DetSetVector<TTStub<Ref_Phase2TrackerDigi_> >, TTStub<Ref_Phase2TrackerDigi_> > > stubRefs = iterL1Track->getStubRefs();
+    int nstub = (int)stubRefs.size();
+    if( chi2rz<3.0 && MVA2>0.2 && MVA1>0.2 && pt>3.0 && fabs(eta)<2.4){


I would recommend moving these track selection criteria to the python config to avoid the hard coding of these criteria here or using the track selection producer

I second using the track selection producer as this is much cleaner. You will just have to create another config in the python config and add whatever varibles are not already there (like MVA2), and then pass this track collection into the displaced vertex config here.

I'm looking at the track selector and I'm wondering if using this is in fact cleaner than simply moving the parameters to a python config as Christopher suggested. The selector producer seems to be made for simpler, single variable cuts while I have several selections that depend on two variables such as the MVA score of displaced (d0>1cm) tracks. The selector also is cutting on all defined variables regardless of which ones the user has overwritten so there are many wasted operations such as checking if reducedBendChi2MaxNstub5<999.9 which would always be true.

I would agree with @Chriisbrown and @cgsavard. You should add these to the python configuration of this module so the values of the cuts can be set there, but keep the logic here. I don't see where wasted checks and operations would happen there?

@aloeliger For example, reducedChi2RZMaxNstub4 is defined which I'm currently not cutting on. This parameter is used as part of the chi2nstub selection which is used for every track here. So this track selection producer seems to cut on every defined variable even though for many of the default values such as this one the cut is essentially a Boolean true

You shouldn't need to set a magnitude size, this is for when a variable needs to be converted from a track word variable. Since d0 is a variable directly included in the track word, no conversion is needed. You should follow what is being done for getting the z0 using the proper d0 functions from the track word class.

I've modified the displaced vertex producer to use the track selection producer here 63cdfcf

Does the MC track association have to be redone after using the track selection producer? The truth association seems to not be working now after this change

No, it shouldn't change anything except for where track selection is being done. The only thing I can think of is some of the files don't have the truth association included so maybe you are looking at one of those files?

I've been using the same file for testing these PR changes and I was able to get the tracking particles before. Now it's giving a null pointer for each track. I looked at the other producers that use the track selection and it seems like they don't use truth information. Has anyone done this before?

Chriisbrown · 2024-05-15T19:54:15Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker_cfg.py

@@ -193,6 +194,16 @@
    process.pTkMHT = cms.Path(process.l1tTrackerHTMiss*process.l1tTrackerHTMissExtended)
    process.pTkMHTEmulator = cms.Path(process.l1tTrackerEmuHTMiss*process.l1tTrackerEmuHTMissExtended)
    process.pL1TrackTripletEmulator = cms.Path(process.l1tTrackTripletEmulation)
+    process.DispVert = cms.Path(process.DisplacedVertexProducer)
+    process.printTree = cms.EDAnalyzer("ParticleTreeDrawer",


Can this be added as an optional part of the ObjectNtupleMaker rather than by default unless it is something that everyone would want running when creating the L1TTrack Objects

I added a boolean in the cfg to run the displaced vertexing which is false by default here d95ffe9

cgsavard · 2024-05-15T20:44:41Z

L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h

+
+using namespace std;
+
+class Track_Parameters


Is it necessary to create this new class? Can you just use the TTTrack class?

It was convenient to define a class which contains the parameters of the projection of the track on the transverse plane just so I only calculate it once per track. Is there a preference for using functions or class methods for this type of thing?

The preference is not necessarily for functions versus class members, but for condensing the number of in use classes to try and consolidate code and functionality.

There are at least a few things conceptually similar between TTrack and it's parent class TTTrack_TrackWord and this track parameters class. It would be good if the track trigger developers could try to centralize as many common elements to most types of tracks in one place if possible

So what would you recommend? I could try inheriting from TTrack in my class and only add the extra functionality I need or I could try to do without an additional class at all.

My recommendation would be to remove this class entirely, pass the L1 track into these functions and use the TTTrack/TTTrack_Word variables directly, and similarly for everywhere else this is used like here.

… when setting track quality score

cgsavard · 2024-05-17T13:29:07Z

L1Trigger/TrackFindingTracklet/plugins/L1FPGATrackProducer.cc

+	  aTrack.settrkMVA2(qualityScore);
+	}
+	else{
+	  aTrack.settrkMVA1(qualityScore);


This is not quite doing what we want. It seems like when extended==false, trkMVA1 is being overwritten with the qualityScore from the displaced model. We should remove the else statement, and only MVA2 is filled if extended_==true.

Also, why is there an additional constraint on the displaced model being a BDT? The displaced model should just be whatever is in the configuration and this seems like it will not let any changes to occur? I think it's better to make all changes to the configs than to the hardset code

Currently only the BDT is defined for displaced tracks, right? I wanted to allow for the use of extended tracking with the prompt classifiers such as the NN. I've also found it useful to use both the prompt and displaced MVA score while using the extended tracking which is why I've set it up this way.

That's right. I think we should always fill MVA1 with the prompt classifier no matter what (so that would be the removal of the else statement) and if someone wants to switch the prompt classifier to another one, say a NN, then that is done in the config. Then whenever extended tracking is run, we should fill MVA2 with the displaced one in addition to the prompt MVA1 one. And if people want to change the displaced one then that should happen in the config, no additional filtering should really be done here.

So here is what I'm imagining:

if (trackQuality_) { aTrack.settrkMVA1(trackQualityModel_->getL1TrackQuality(aTrack)); if(extended_) aTrack.settrkMVA2(trackQualityDispModel_->getL1TrackQuality(aTrack));

Okay, I've made the fix d95ffe9

…changes to ntuple maker params, removing BDT requirement on displaced track quality

triggerDoctor · 2024-05-20T08:20:22Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-05-20T08:57:26Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found issues with the headers. These issues likely arise from improper build files.

The following is the stderr of the header checking:

In file included from src/L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h:12:
/cvmfs/cms.cern.ch/el9_amd64_gcc12/cms/cmssw/CMSSW_14_0_0_pre3/src/PhysicsTools/ONNXRuntime/interface/ONNXRuntime.h:19:10: fatal error: onnxruntime/core/session/onnxruntime_cxx_api.h: No such file or directory
   19 | #include "onnxruntime/core/session/onnxruntime_cxx_api.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
gmake: *** [config/SCRAM/GMake/Makefile.rules:1833: tmp/el9_amd64_gcc12/src/L1Trigger/L1TTrackMatch/src/L1TriggerL1TTrackMatch/DisplacedVertexProducer.h.chk_header] Error 1
gmake: Target 'check-headers' not remade because of errors.

Info	Value
return code	`2`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-05-20T08:58:03Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 11 files that did not meet formatting requirements:

DataFormats/L1Trigger/interface/DisplacedVertex.h
L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc
L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h
L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc
L1Trigger/L1TTrackMatch/test/Analyzer_DisplacedMuon.C
L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc
L1Trigger/Phase2L1GMT/interface/TPSLUTs.h
L1Trigger/Phase2L1GMT/plugins/Phase2L1TGMTFwdMuonTranslator.cc
L1Trigger/TrackFindingTracklet/plugins/L1FPGATrackProducer.cc
L1Trigger/TrackTrigger/interface/L1TrackQuality.h
L1Trigger/TrackTrigger/src/L1TrackQuality.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

aloeliger · 2024-05-20T14:36:36Z

L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h

+  if(a.charge>0){
+    p_vec *= -1;
+  }
+  p_vec /= TMath::Sqrt(pow(p_vec[0],2)+pow(p_vec[1],2));


Is it possible that the arguments to this vector will end up as 0? This will causes errors if so.

Added a check for this in this commit 63cdfcf

aloeliger · 2024-05-20T14:37:24Z

L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h

+    std::valarray<float> p_trk_2 = calcPVec(b_in,x_dv_in,y_dv_in);
+    std::valarray<float> p_tot = p_trk_1+p_trk_2;
+    p_mag = TMath::Sqrt(pow(p_tot[0],2)+pow(p_tot[1],2));
+    openingAngle = (p_trk_1[0]*p_trk_2[0]+p_trk_1[1]*p_trk_2[1]) / (TMath::Sqrt(pow(p_trk_1[0],2)+pow(p_trk_1[1],2))*TMath::Sqrt(pow(p_trk_2[0],2)+pow(p_trk_2[1],2)));


Same question about possible division by zero errors here.

Added a check for this in this commit 63cdfcf

aloeliger · 2024-05-20T14:37:39Z

L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h

+    p_mag = TMath::Sqrt(pow(p_tot[0],2)+pow(p_tot[1],2));
+    openingAngle = (p_trk_1[0]*p_trk_2[0]+p_trk_1[1]*p_trk_2[1]) / (TMath::Sqrt(pow(p_trk_1[0],2)+pow(p_trk_1[1],2))*TMath::Sqrt(pow(p_trk_2[0],2)+pow(p_trk_2[1],2)));
+    R_T = TMath::Sqrt(pow(x_dv_in,2)+pow(y_dv_in,2));
+    cos_T = (p_tot[0]*x_dv_in+p_tot[1]*y_dv_in)/(R_T*TMath::Sqrt(pow(p_tot[0],2)+pow(p_tot[1],2)));


Added a check for this in this commit 63cdfcf

aloeliger · 2024-05-20T14:38:25Z

L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h

+
+class Vertex_Parameters
+{
+public:


Are there any extant Track Trigger classes for handling vertexes that would be good to centralize this class with?

My personal preference would be to get rid of this class as well and use these variables directly in the DisplacedVertexProducer class to avoid another vertex-like class, but I also see why you may want to set it up like this. It doesn't quite follow how the other GTT object classes are set up so I would suggest maybe changing this class to a struct instead as it's really only holding variables to be used by DisplacedVertexProducer.

There is no vertex class that has all of these variables.

can this (class => functions) be done as a "code improvement" in a second stage? L1 is requesting that this PR is merged in order for @ryanm124 to show the plots at an upcoming conference. that request is to have code to be able to reproduce the results, and it seems code improvements wouldn't be necessary for that?

This is ok with me for this comment. There does seem to be some potential logic issues brought up in other comments that might need more addressing. And at minimum, the code checks and formatting should be done and It doesn't seem like the formatting has been applied. You should run these two commands:

scram build code-checks scram build code-format

Code formatting has been applied 93ea36d

Is there any update on this given that the conference has passed, and so the "code improvements" can be implemented?

aloeliger · 2024-05-20T14:40:56Z

L1Trigger/L1TTrackMatch/interface/DisplacedVertexProducer.h

+class DisplacedVertexProducer : public edm::global::EDProducer<> {
+ public:
+  explicit DisplacedVertexProducer(const edm::ParameterSet&);
+  ~DisplacedVertexProducer() override {}


Suggested change

~DisplacedVertexProducer() override {}

~DisplacedVertexProducer() override = default;

I've implemented this

aloeliger · 2024-05-20T15:06:33Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+    float MVA2 = iterL1Track->trkMVA2();
+    std::vector<edm::Ref<edmNew::DetSetVector<TTStub<Ref_Phase2TrackerDigi_> >, TTStub<Ref_Phase2TrackerDigi_> > > stubRefs = iterL1Track->getStubRefs();
+    int nstub = (int)stubRefs.size();
+    if( chi2rz<3.0 && MVA2>0.2 && MVA1>0.2 && pt>3.0 && fabs(eta)<2.4){


I would agree with @Chriisbrown and @cgsavard. You should add these to the python configuration of this module so the values of the cuts can be set there, but keep the logic here. I don't see where wasted checks and operations would happen there?

aloeliger · 2024-05-20T15:09:41Z

L1Trigger/L1TTrackMatch/test/Analyzer_DisplacedMuon.C

+  //preselection cuts and plots definitions
+  // Cut assumptions: first cut is maxEta
+  std::vector<std::unique_ptr<Cut>> preselCuts;
+  std::unique_ptr<TypedCut<float>> cut0(new TypedCut<float>("maxEta","max #eta",&trk_eta,2.4,true));


I know this is test code and isn't really intended for main CMSSW use, but std::make_unique is probably preferred.

aloeliger · 2024-05-20T15:19:14Z

L1Trigger/L1TTrackMatch/test/Analyzer_DisplacedMuon.C

Does this need to go into CMSSW? It seems like a large single purpose macro, designed for one person or group only.

No it doesn't, I can remove it from the PR

aloeliger · 2024-05-20T15:21:56Z

L1Trigger/L1TTrackMatch/test/trackAndVert_NewTQMVA_model.onnx

This cannot be committed to CMSSW. It's fine here I guess, but in a PR to CMSSW all non-code files need to be committed to the data repositories.

aloeliger · 2024-05-20T15:23:02Z

L1Trigger/TrackTrigger/data/clf_GBDT_newbin_disp_xgb.onnx

This cannot be committed to CMSSW. It's fine here I guess, but in a PR to CMSSW all non-code files need to be committed to the data repositories.

ryanm124 · 2024-07-17T09:11:16Z

The track selection module has broken the truth association. I suspect it's changing the addresses of the tracks which breaks the mapping between the tracks and tracking particles. Can I move the cuts back to my displaced vertex producer but have the cut values be input from the python config? Otherwise, I need someone to help get this working

…ed bug in track truth association which affected BDT training boolean

ryanm124 · 2024-08-28T08:15:24Z

I've removed the track selection module and moved the cut values to the python configuration in my last commit. The code is working and should be ready to merge

…fixed<13,8>, shifting quality variables, and rescaling delta z and pt

ryanm124 · 2024-11-14T16:05:38Z

I've added the emulation of my displaced vertexing to this PR. I need to remove some changes/files which are no longer necessary and then this PR needs to be rereviewed

…k quality

ryanm124 · 2024-11-18T12:01:02Z

@cgsavard @aloeliger @Chriisbrown @skinnari I've finished updating the PR to include the emulation of the displaced vertexing. I've removed the changes I had made to the track selection and to the displaced track quality as they're no longer needed. Could you please rereview this PR?

Chriisbrown · 2025-01-30T13:54:19Z

DataFormats/L1TrackTrigger/src/TTTrack_TrackWord.cc

This change is already in central cmssw, should this PR be rebased to a newer CMSSW release?

Good idea, I'll rebase my central CMSSW PR

Chriisbrown · 2025-01-30T14:13:22Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+    edm::Ptr<TTTrack<Ref_Phase2TrackerDigi_>> l1track_ref(TTTrackHandle, this_l1track);
+    this_l1track++;
+
+    float pt = FloatPtFromBits(*l1track_ptr);


What was the reason for casting things to float and needing seperate functions to extract various parameters, why can't the internal representations in L1TrackUnpacker.h?

I was following the example of the Track Triplet Emulator which Benjamin had recommended since it was merged: https://github.com/gkaratha/cmssw/blob/ff352864a0da1cc97490466b89b6f70deeba07f2/L1Trigger/L1TTrackMatch/plugins/L1TrackTripletEmulatorProducer.cc#diff-a82ab3ec6f47ebc414fc95786550558ba75df8d73d6f91c91652c348783701df. Converting the ap_ints to floats was simpler so I could use the same functions and cut parameters as the simulation which is run in parallel with the emulation

Chriisbrown · 2025-01-30T14:14:33Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+                                                                         vertex.p_mag,
+                                                                         isReal);
+
+      std::vector<float> Transformed_features = {float(selectedTracksWithTruth[i].first.pt*0.25),


what is the 0.25 multipication?

I'm shifting the input features so they all fall within a similar range to minimize the amount of bits needed by the BDT

Chriisbrown · 2025-01-30T14:14:46Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+                                                 selectedTracksWithTruth[j].first.d0,
+                                                 selectedTracksWithTruth[i].first.z0,
+                                                 selectedTracksWithTruth[j].first.z0,
+                                                 float(selectedTracksWithTruth[i].first.chi2rz+0.07),


what is the 0.07 addition?

This was my hack to get around the conifer conversion bug. Shifting the quality parameters by a small amount allowed the comparisons with the cutoff values in the BDT to function properly

Chriisbrown · 2025-01-30T14:15:03Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+                                                 vertex.d_T,
+                                                 vertex.R_T,
+                                                 vertex.cos_T,
+                                                 float(vertex.delta_z*0.125)};


What is this 0.125 multiplication?

I'm shifting the input features so they all fall within a similar range to minimize the amount of bits needed by the BDT

Chriisbrown · 2025-01-30T14:17:14Z

L1Trigger/L1TTrackMatch/plugins/DisplacedVertexProducer.cc

+
+      conifer::BDT<ap_fixed<13,8,AP_RND_CONV, AP_SAT>, ap_fixed<13,8,AP_RND_CONV, AP_SAT>> bdt(this->model_);
+      std::vector<ap_fixed<13,8,AP_RND_CONV, AP_SAT>> output = bdt.decision_function(Transformed_features);
+      outputVertex.setScore(output.at(0).to_float());


Why is this set as a float?

It's using the same vertex class and methods as the simulation which expects a float. Also the ntuple maker would have to convert the score to a float anyway before storing in a TBranch, right?

Chriisbrown · 2025-01-30T14:19:19Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc

+  int ChargeFromBits(const L1Track&) const;
+  double convertPtToR(double pt);
+
+  inline const unsigned int DoubleToBit(double value, unsigned int maxBits, double step) const {


These are already defined in L1TrackUnpacker.h?

Chriisbrown · 2025-01-30T14:20:40Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc

@@ -284,12 +330,50 @@ class L1TrackObjectNtupleMaker : public edm::one::EDAnalyzer<edm::one::SharedRes
  std::vector<float>* m_gen_z0;
  std::vector<float>* m_gen_mother_pdgid;

+  //displaced vertices


Do all of these need to be added to the ntuples or can you derive most of these in your training code? How much are these variables specific to your use case or are they general for many different algorithm developments?

The bare minimum would be the two track indices, the vertex score, and the isReal boolean. The rest could be calculated by remaking the vertex from the two tracks. What I would prefer to do is just remove unused variables (opening angle, parent pt, inTraj, dvEmuFixed branches) and then consolidate the dv and dvEmu branches into one set of branches which is filled with the simulated or emulated values depending on a boolean. That would leave 11 branches which are only saved if a user turns the displaced vertexing on and they would not have to recalculate the vertices. Would that be acceptable?

Chriisbrown · 2025-01-30T14:28:04Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc

@@ -994,6 +1241,119 @@ void L1TrackObjectNtupleMaker::endJob() {
  delete m_trkfastjetExt_truetp_sumpt;
 }

+template <typename T>


Do these functions need to be in this ntuple maker or can you do this in your secondary analysis as typically there aren't specific signatures being targeted by this ntuple maker

I can remove the isHtoMu and isHtoB as they're not needed anymore. The isHard function is just checking pythia flags which is agnostic to the signature, so I'd like to keep that one function

Chriisbrown · 2025-01-30T14:30:20Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc

@@ -3055,6 +3920,27 @@ void L1TrackObjectNtupleMaker::analyze(const edm::Event& iEvent, const edm::Even
      m_matchtrkExt_lhits->push_back(tmp_matchtrkExt_lhits);
      m_matchtrkExt_seed->push_back(tmp_matchtrkExt_seed);
      m_matchtrkExt_hitpattern->push_back(tmp_matchtrkExt_hitpattern);
+
+      m_matchtrkExtEmu_pt->push_back(tmp_matchtrkExtEmu_pt);


Again, can this be done in external analysis scripts?

I thought saving the emulated extended tracks would be general enough to include in the ntuple maker. Wouldn't it be better to unpack and store these values here rather than everyone writing their own EDAnalyzer to get these emulated values? I can remove the branches which use my quantization, the "trkExtEmuFixed" branches, but the "trkExtEmu" should be useful to anyone using emulated tracks

Chriisbrown · 2025-01-30T14:32:36Z

L1Trigger/TrackFindingTracklet/python/l1tTTTracksFromTrackletEmulation_cfi.py

@@ -37,6 +37,6 @@
                                               tableTEDFile = cms.FileInPath('L1Trigger/TrackFindingTracklet/data/table_TED/table_TED_D1PHIA1_D2PHIA1.txt'),


By default this is off for the track finder, if that is how the track finder have it leave it in their default, if this has been updated in more recent CMSSW releases then rebase to the newer release

Chriisbrown · 2025-01-30T14:33:12Z

L1Trigger/TrackFindingTracklet/python/l1tTTTracksFromTrackletEmulation_cfi.py

@@ -37,6 +37,6 @@
                                               tableTEDFile = cms.FileInPath('L1Trigger/TrackFindingTracklet/data/table_TED/table_TED_D1PHIA1_D2PHIA1.txt'),


By default this is off for the track finder, if that is how the track finder have it leave it in their default, if this has been updated in more recent CMSSW releases then rebase to the newer release

aloeliger · 2025-02-05T15:32:28Z

@ryanm124 Could I ask that any changes made on this PR, and any changes for review be moved to the CMSSW PR cms-sw#45672. I would like to close this out as completed, and not let things get lost here.

ryanm124 · 2025-02-07T08:49:35Z

@ryanm124 Could I ask that any changes made on this PR, and any changes for review be moved to the CMSSW PR cms-sw#45672. I would like to close this out as completed, and not let things get lost here.

Sure, I'll update cms-sw#45672 to include the emulation and will address Chris's comments there

ryanm124 added 4 commits May 8, 2024 14:14

Adding Displaced Vertexing Producer

0255f1a

Modifying track quality MVA inputs for displaced tracks

24413dc

Reverting changes to prompt track quality BDT, adding fix to MVA scor…

acde98b

…e, and updating displaced vertex BDT

Adding newest BDT model file and displaced vertexing analyzer

0dde2bd

cgsavard reviewed May 14, 2024

View reviewed changes

Chriisbrown reviewed May 15, 2024

View reviewed changes

cgsavard reviewed May 15, 2024

View reviewed changes

Adding back tanL to prompt MVA and adding check for extended tracking…

dbf8525

… when setting track quality score

cgsavard reviewed May 17, 2024

View reviewed changes

Adding option to run displaced vertexing in ntupler maker, reverting …

d95ffe9

…changes to ntuple maker params, removing BDT requirement on displaced track quality

aloeliger reviewed May 20, 2024

View reviewed changes

aloeliger added Phase-2 Pertains to phase-2 development Algorithm Development labels May 20, 2024

ryanm124 mentioned this pull request May 22, 2024

Adding displaced track quality model file cms-data/L1Trigger-TrackTrigger#4

Open

Removing track selection module and moving cuts to python config. Fix…

bff29e9

…ed bug in track truth association which affected BDT training boolean

ryanm124 added 7 commits September 18, 2024 10:38

Adding RT cut to producer and adding isHard boolean to ntuple

0a9fccc

Adding displaced vertexing emulation

a9e384e

Adding file to convert xgboost model to conifer

7e435db

Adding plotting script

41431b6

Final changes for emulation, changing bit widths for BDT input to ap_…

3245d7a

…fixed<13,8>, shifting quality variables, and rescaling delta z and pt

Adding do plot check for vertex plots

6459a2c

Cleaning up code

61a3c52

ryanm124 added 3 commits November 14, 2024 17:22

Deleting unused models and scripts

a71a93f

Moving BDT to seperate PR in data directory

b4ed6f3

Removing track selection producer changes and removing displaced trac…

04c75c3

…k quality

ryanm124 mentioned this pull request Jan 15, 2025

Displaced vertexing gbdt cms-sw/cmssw#45672

Open

Chriisbrown reviewed Jan 30, 2025

View reviewed changes

	~DisplacedVertexProducer() override {}
	~DisplacedVertexProducer() override = default;

		@@ -37,6 +37,6 @@
		tableTEDFile = cms.FileInPath('L1Trigger/TrackFindingTracklet/data/table_TED/table_TED_D1PHIA1_D2PHIA1.txt'),

GBDT-Based Displaced Vertex Producer #1257

Are you sure you want to change the base?

GBDT-Based Displaced Vertex Producer #1257

Conversation

ryanm124 commented May 14, 2024 • edited Loading

triggerDoctor commented May 14, 2024

triggerDoctor commented May 14, 2024

triggerDoctor commented May 14, 2024

triggerDoctor commented May 14, 2024

triggerDoctor commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chriisbrown May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triggerDoctor commented May 20, 2024

triggerDoctor commented May 20, 2024

triggerDoctor commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanm124 commented Jul 17, 2024

ryanm124 commented Aug 28, 2024

ryanm124 commented Nov 14, 2024

ryanm124 commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanm124 commented May 14, 2024 •

edited

Loading

Chriisbrown May 15, 2024 •

edited

Loading

ryanm124 Feb 7, 2025 •

edited

Loading