Added T3/T5 branches for GNN T5 classification #250

jkguiang · 2023-03-03T22:39:45Z

Added GNN NTuple branches for T5 classification plus some minor fixes to Event.cu

jkguiang · 2023-03-03T22:40:19Z

Let me know if I should address #249 in this PR as well. I think the fix is easy.

VourMa

Since two new variables are added (are they used anywhere, by the way?), could you please run the checks here to be sure that everything checks out?

VourMa · 2023-03-05T20:50:34Z

code/core/write_sdl_ntuple.cc

-    }
+    std::set<unsigned int> T3s_used_in_T5;
+    std::map<unsigned int, unsigned int> T3_index_map;
+    // std::map<unsigned int, unsigned int> T5_index_map; // not used


If not used, maybe it should be deleted?

VourMa · 2023-03-05T20:51:17Z

code/core/write_sdl_ntuple.cc

+        /* We use getMDsFromLS instead, as we only want MDs associated w/ LS candidates
+        // Loop over minidoublets
+        nTotalMD += miniDoubletsInGPU.nMDs[idx];
+        for (unsigned int jdx = 0; jdx < miniDoubletsInGPU.nMDs[idx]; jdx++)
+        {
+            // Get the actual index to the mini-doublet using rangesInGPU
+            unsigned int mdIdx = rangesInGPU.miniDoubletModuleIndices[idx] + jdx;
+            setGnnNtupleMiniDoublet(event, mdIdx);
+        }
+        */


This also seems to be unused, so better delete it?

VourMa · 2023-03-05T20:56:47Z

code/core/write_sdl_ntuple.cc

+    ana.tx->pushbackToBranch<float>("t3_4_z", hit4_z);
+
+    /* Sigmas for chi2 calculation 
+     * (stolen from SDL::computeSigmasForRegression and SDL::computeRadiusUsingRegressionk) */


I know that this is copied code but I wouldn't just blindly copy it, so I have a few questions below...

VourMa · 2023-03-05T20:58:31Z

code/core/write_sdl_ntuple.cc

+    float inv1 = 0.01f/0.009f;
+    float inv2 = 0.15f/0.009f;


What do these represent?

VourMa · 2023-03-05T20:58:44Z

code/core/write_sdl_ntuple.cc

+    std::vector<float> sigmas;
+    float inv1 = 0.01f/0.009f;
+    float inv2 = 0.15f/0.009f;
+    // float inv3 = 2.4f/0.009f; // not used


Delete, if unused?

VourMa · 2023-03-05T21:00:25Z

code/core/write_sdl_ntuple.cc

+        // Category 1: barrel PS flat
+        if (module_subdet == SDL::Barrel and module_type == SDL::PS and module_side == SDL::Center)
+        {
+            delta1 = inv1;//1.1111f;//0.01;


Commented out numbers should be deleted? The first one seems to be the result of inv1 (so why have it re-written), while the second one is random - at least I can't understand where it comes from...

This comment applies to similar instances below.

VourMa · 2023-03-05T21:02:19Z

code/core/write_sdl_ntuple.cc

+            /* Despite the type of the module layer of the lower module index,
+             * all anchor hits are on the pixel side and all non-anchor hits are
+             * on the strip side! */


Is this comment relevant here? If it is, probably its connections should be made clearer?

VourMa · 2023-03-05T21:04:38Z

code/core/write_sdl_ntuple.cc

+    const float kRinv1GeVf = (2.99792458e-3 * 3.8);
+    const float k2Rinv1GeVf = kRinv1GeVf / 2.;


Should we just include the constants from here?

VourMa · 2023-03-05T21:05:20Z

code/core/write_sdl_ntuple.cc

+    float betaIn  = __H2F(tripletsInGPU.betaIn[T3]);
+    float betaOut = __H2F(tripletsInGPU.betaOut[T3]);
+
+    // Legacy T4 pt estimate


T4? I guess this is confusing, because we don't have T4s now?

VourMa · 2023-03-05T21:06:46Z

code/core/write_sdl_ntuple.cc

@@ -1042,7 +1292,7 @@ std::tuple<float, float, float, vector<unsigned int>, vector<unsigned int>> pars
    const float ptAv_out = abs(dr_out * k2Rinv1GeVf / sin((betaIn_out + betaOut_out) / 2.));

    // T5 pt is average of the two pt estimates
-    const float pt = (ptAv_in + ptAv_out) / 2.;
+    const float pt = (ptAv_in + ptAv_out) / 2.;     // this is deprecated, c.f. setQuintupletOutputBranches


Not sure I understand the comment. Is this used? If not, maybe we should delete it and use the up-to-date version?

sgnoohc · 2023-03-12T21:29:56Z

general comment.
Is there a reason why not to put idxs to the relevant LS in the t3 branches?
The flattening of the t3 hit positions are OK in principle.
But it puts a danger in creating a lot of branches and lots of duplicate information.
(e.g. if two T3's share a LS, it's going to copy paste same information twice.)

I would rather do something like
T3_LS_idx0
keeping the same branch philosophy as the LS.

sgnoohc · 2023-03-12T21:37:53Z

code/core/write_sdl_ntuple.cc

@@ -181,6 +181,30 @@ void createGnnNtupleBranches()
    ana.tx->createBranch<vector<float>>("LS_sim_vz");
    ana.tx->createBranch<vector<int>>("LS_isInTrueTC");

+    // T3 branches
+    ana.tx->createBranch<vector<int>>("t5_t3_idx0");
+    ana.tx->createBranch<vector<int>>("t5_t3_idx1");


parallelism is broken here.
Technically these are t5 branches.
The comment //T3 branches showing right above this is misleading

What do you mean by "parallelism is broken"? The comment is a typo and will be fixed with the responses to Manos's comments.

jkguiang · 2023-03-13T16:10:11Z

Is there a reason why not to put idxs to the relevant LS in the t3 branches?

We use the three hits stored in the t3 branches for computing Balaji's Chi^2. Storing them directly makes it very easy to use them as inputs to the GNN, rather than requiring nested lookups (first to the LS, then to the MD). I also figure that there is not much "danger" here for now, as this is just for training the GNN.

I would rather do something like
T3_LS_idx0
keeping the same branch philosophy as the LS.

This is doable if necessary, but would be more of a headache as explained above.

jkguiang added 7 commits March 1, 2023 10:06

initial commit for GNN T3/T5 output branches

ea3c7a0

fixed

19c3abb

added missing lines for modulesInCPUFull->eta and modulesInCPUFull->r

75810e7

added lines to store sigmas used in T5 chi2 regression

1f0d9a9

cleaned up

849a867

fixed

c652b67

Merge branch 'SegmentLinking:master' into master

17a255b

renamed T3 legacy pt branch

1905806

VourMa reviewed Mar 5, 2023

View reviewed changes

sgnoohc reviewed Mar 12, 2023

View reviewed changes

jkguiang closed this May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added T3/T5 branches for GNN T5 classification #250

Added T3/T5 branches for GNN T5 classification #250

jkguiang commented Mar 3, 2023

jkguiang commented Mar 3, 2023

VourMa left a comment

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

VourMa Mar 5, 2023

sgnoohc commented Mar 12, 2023

sgnoohc Mar 12, 2023

jkguiang Mar 13, 2023

jkguiang commented Mar 13, 2023

		const float kRinv1GeVf = (2.99792458e-3 * 3.8);
		const float k2Rinv1GeVf = kRinv1GeVf / 2.;

Added T3/T5 branches for GNN T5 classification #250

Added T3/T5 branches for GNN T5 classification #250

Conversation

jkguiang commented Mar 3, 2023

jkguiang commented Mar 3, 2023

VourMa left a 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

sgnoohc commented Mar 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkguiang commented Mar 13, 2023