Implementation of TS clustering and tracking as envisioned in LUT firmware.

[rodwyer100]

This push request takes the validated HLS firmware code and reincorporates it into the ldmx-sw structure. I have included in TrigScintFirmwareProducer a producer which takers clusterproducer_sw and trackproducer_sw (faithful reproductions of the code used to implement tracking in HLS), stages a collection of digis for insertion, and then obtains a collection of tracks for use thereafter. The producer as of yet does not save the collected digis into the track object, nor does it produces clusters, but it does faithfully reproduce the tracks of the existing horizontal geometry. There are two caveats with this:

1. It completely reproduces ldmx reconstruction results with no inefficiency in HLS because HLS does a better job at staging the rather poorly behaves ap_int classes it uses. I had some initial issues getting this code to be 100% efficient (its 98% efficient now) because ap_int behaves irregularly in how its implemented here.
2. This Firmware design was used to implement the original horiztonal 3 pad geometry (ldmx-det-v14) and does not have a LUT that corresponds to vertical bars. If this is desired, one could envision changing trackproducer_hw and testing it, but it will require further work.

• I successfully compiled ldmx-sw with my developments
• I ran my developments and the following shows that they are successful.

I validated that the producer made the same number of tracks using print out statements like this:
10 10 10
18 18 18
29 29 30
[ ElectronCounter ] 0 : Found 3 electrons (tracks) using input collection TriggerPadTracks_sim
I have not validated this code as extensively as the firmware version, so it could presumably make tracks from different clusters but my intuition about how the code works tells me that is very unlikely.

Here is a picture of the track number distribution for both:
Track no for original reconstruction:

Track no for firmware reconstruction:

If any more plots are necessary please lmk

[tvami]

I am adding some includes in /includes/ which are necessary for the primitive non floating point objects that HLS uses to create firmware from C++ files.

How are these files different from what we have in Trigger/HLS_arbitrary_Precision_Types/include? If they are not you should not add them again, but rather reuse them

[tvami]

I have a few smaller comments, please generally clean up the files, and you'll also have to run just format-cpp to have the clang formatting. I can also run the other sanitizers tomorrow for more comments, or maybe Tom will catch those before I run it tomorrow.

[rodwyer100]

We should talk about how ap_int is implemented in the ldmx-sw framework. I know TS in the Trigger repository doesn't use it, and I suspect it had something to do with the <> in the fixed point library requiring ap_int to be found as a standard directory (and therefore not being found). While there are a couple repositories in the Ecal Trigger repositories that use it I am not certain how Christian managed to work with the relative paths indicated by the use of quotation marks and still have it built with <>. That doesn't seem to work for me, though Im happy to take suggestions. I have implemented a fix that should work for both of us, and that is that in my local copy I have all of the includes of the HLS objects switched (wherever relevant) from <> to "". The issue is that since these are contained in a submodule we don't have access to, I can push that version which would work for us both. I know Tom worked on dereferening TrigScint and I am wondering if that is called for here, or if there is something better one should do. Otherwise I am not certain how to proceed on that front.

[bryngemark]

I have a general structure/philosophy comment/question. First, are these meant to be used in the HLS implementation or in sw clustering/tracking for comparison to HLS? If the former, I wonder if we should put them in some separate directory (be it within TrigScint or Trigger, I don't know). If the latter, I'd like the producer naming to be more explicit. You do use one explicit name "TrigScintFirmwareTracker" and that model is great. Overall it would be nice if you follow e.g. the capitalization conventions we're used to.

I would also appreciate if you commented the code more, so the next person can follow what's going on.

[bryngemark]

should these be the QIEDigis?

[rodwyer100]

I will comment the code more thoroughly for sure :)! I am including this code for two reasons. The first is that Elizabeth needs the code for efficiency studies to reflect firmware stuff. The other reason is along the lines of having firmware directly reflect software, and since the firmware uses pattern matching so will the software. This processor does both clustering and tracking in one step, using clusterproducer_sw and trackerproducer_hw to do those steps respectively. These files are the raw ones that are the validated pieces of HLS code, the processor itself just stages the digis. If I wanted to include the hitmaker I could as well, in which case I guess the input would be adc and tdc stuff from QIEDecoder, but I haven't done that just yet.

[tvami]

The issue is that since these are contained in a submodule we don't have access to,

I'm confused by this statement. Doesn't this mean you just forgot the "recursive" when cloning in git clone --recursive git@github.com:LDMX-Software/ldmx-sw.git ? It should really be available like that

[tomeichlersmith]

Christian got the relative path includes working by telling CMake where to find the necessary headers.

ldmx-sw/Trigger/Algo/CMakeLists.txt

Line 39 in 77fffe1

target_include_directories(Trigger PUBLIC ../HLS_arbitrary_Precision_Types/include)

This type of thing would be necessary in TrigScint as well if the ap_* headers are of use. Without a line like this in the CMake, there would be no way for the compiler to figure out where the headers are and thus would emit a unable to find ap_XXX.h style compiler error.

[tomeichlersmith]

I did a quick test and this branch still compiles after the following changes.

git rm -r TrigScint/include/TrigScint/etc

diff --git a/TrigScint/CMakeLists.txt b/TrigScint/CMakeLists.txt
index 2d1a8473..05382878 100644
--- a/TrigScint/CMakeLists.txt
+++ b/TrigScint/CMakeLists.txt
@@ -44,6 +44,7 @@ setup_library(module TrigScint
               dependencies Framework::Framework Recon::Event DetDescr::DetDescr
                            Tools::Tools SimCore::Event
 )
+target_include_directories(TrigScint PUBLIC ../Trigger/HLS_arbitrary_Precision_Types/include)
 
 setup_python(package_name LDMX/TrigScint)
 
diff --git a/TrigScint/include/TrigScint/objdef.h b/TrigScint/include/TrigScint/objdef.h
index 230cf112..b9706db9 100755
--- a/TrigScint/include/TrigScint/objdef.h
+++ b/TrigScint/include/TrigScint/objdef.h
@@ -1,7 +1,7 @@
 #ifndef OBJDEF_H
 #define OBJDEF_H
 
-#include "../../../Trigger/HLS_arbitrary_Precision_Types/include/ap_int.h"
+#include "ap_int.h"
 #define NTIMES 6
 #define NHITS 25
 #define NCLUS 25

[rodwyer100]

I have added every change (smart pointers, general adoption of existing capitalization structure, etc) with the exception of clang formatting. I am trying to get my clang formatter working, but s3df doesnt allow for sudo so I have to git clone it and make it and Im on hour 6 or 7 of that. So hopefully it will all be done tomorrow. Please lmk if the chosen way of aligning with the capitalization structure is sufficient!

[tvami]

@rodwyer100 after we merge #1464 the clang format will automatically be applied if it's too much work for you. (Tho I use s3df too and just format-cpp works really nicely w/o any sudo needs)

[rodwyer100]

The files have been clang formatted. Lmk if you think it needs anything else :)!

[tvami]

The files have been clang formatted. Lmk if you think it needs anything else :)!

The only thing missing from my review is #1461 (comment)

[rodwyer100]

Whoopsie just saw your comment. Fixed that change now. Thanks so much!

[tvami]

It's good from my side now, one more thing to change is the PR description, now that's not in line with the changes since you didnt move the headers in there, please modify the text accordingly

[tomeichlersmith]

I have one technical question and a technical change request.

I haven't looked through to understand the algorithm, but that's what future testing and bug reports are for I suppose.

[tvami]

(not directly connected to this PR, but I'm happy to see the bot is doing the clang update as expected, Rory dont be surprised for this extra commit in your branch)

[tomeichlersmith]

Thank you :) looks good