Conversation
…nd half-integer values without weighting.
Validation ResultsSome validation samples failed! ❌
|
|
Hey Lucia - I thought I'd make a few intertwined, higher-level suggestions that wouldn't fit properly into the line-by-line code review. Currently, your commit implements the LUT production in *You might be wondering 'why would I only need to make one LUT for each geometry, when the probability threshold can be varied?' Well, I see an opportunity to streamline the functionality by including the probability of each combination as a 5th parameter in the LUT, then do the threshold cut once on process start in the @bryngemark may have different thoughts on the matter, so I think it would be useful to hear a second opinion before making large-scale changes to your pull request. |
bryngemark
left a comment
bryngemark
left a comment
There was a problem hiding this comment.
This looks good overall, most of my comments are on formatting etc. I have two general requests:
- add more comments explaining what the code does, either at the top of files/functions or as you go along, as needed
- condense vertical white space a bit. there is no need for blank lines between sets of variables etc, and it makes the code long and hard to read (you have to scroll). NOTE if this white space was added by the formatting script then nevermind
| triplets.output_collection = "clusters.txt" | ||
|
|
||
| p.sequence = [ | ||
| #*truth_hits, |
There was a problem hiding this comment.
if you want these commented here as an indication that including them is optional, i suggest either
- adding a comment saying something like "uncomment to make truth tables" (but i think you need some more stuff for that to work, right? you'd need to define
truth_hitsand use them as input collection to the later processors (at least digi)) - adding a more elaborate setup which does all of the needed steps to use truth hits, and toggle with some boolean, doing {if [that boolean], define stuff, p.sequence.append(truth_hits), then digi etc}
| from LDMX.Framework import ldmxcfg | ||
|
|
||
| p = ldmxcfg.Process("LUTmaker") | ||
| p.input_files = ["Clusters.root"] #necessary to define but unused, the real input file is defined below |
There was a problem hiding this comment.
I see what @cjbarton151 means here: you're not really using ldmx-sw capabilities to manage ROOT tree input and output objects for this, but rather an auxiliary file structure where you have a messy input list and an ordered output list. So in that sense he is right that it's more of a tool and I think moving these files (stuff not reading/writing TS info from/to a tree) to Tools is a good idea. Reach out to one of us if this seems too complicated and we can discuss
| lut_tracks.delta_max = 1.0 | ||
| lut_tracks.verbosity = 1 | ||
| lut_tracks.lut_tracking = True | ||
| lut_tracks.lut_file = "LUT.txt" | ||
| lut_tracks.output_collection = "lutTracks" |
There was a problem hiding this comment.
do these settings differ from the default ones specified in trig_scint.py? if not, i suggest omitting them
| /** | ||
| * @file ClusterTripletMaker.h | ||
| * @brief Writes cluster combinations to text file | ||
| * @author Lucia Kvarnstrom, Lund University |
| std::vector<std::string> cluster_input_collections_; | ||
|
|
||
| // output text file | ||
| std::string output_collection_{"clusters.txt"}; |
There was a problem hiding this comment.
can you name it something else than collection since it's not going to be on a ROOT TTree ?
| if (centroid >= vert_bar_start_idx_ && | ||
| seed.getCentroidY() < cluster1.getCentroidY()) { | ||
| // impossible combination | ||
| if (verbose_ > 1) { |
There was a problem hiding this comment.
remove verbosity check and promote logger level to warning
| cluster1.getCentroidY() > | ||
| cluster2.getCentroidY())) { // impossible | ||
| if (verbose_ > 1) { | ||
| ldmx_log(debug) |
| 3 * max_delta_) && | ||
| (track.getCentroid() < | ||
| vert_bar_start_idx_)) // for the horizontal bars |
There was a problem hiding this comment.
| 3 * max_delta_) && | |
| (track.getCentroid() < | |
| vert_bar_start_idx_)) // for the horizontal bars | |
| 3 * max_delta_) && (track.getCentroid() < vert_bar_start_idx_)) // for the horizontal bars |
| if (((fabs((tracks_.at(idx)).getResidualX() - | ||
| (tracks_.at(idx_comp)).getResidualX())) < | ||
| 0.01) // it should be equal | ||
| && | ||
| (track.getCentroid() >= | ||
| vert_bar_start_idx_)) { // specific case for the vertical bars |
There was a problem hiding this comment.
| if (((fabs((tracks_.at(idx)).getResidualX() - | |
| (tracks_.at(idx_comp)).getResidualX())) < | |
| 0.01) // it should be equal | |
| && | |
| (track.getCentroid() >= | |
| vert_bar_start_idx_)) { // specific case for the vertical bars | |
| if (((fabs((tracks_.at(idx)).getResidualX() - | |
| (tracks_.at(idx_comp)).getResidualX())) < 0.01) // should be equal | |
| && (track.getCentroid() >= vert_bar_start_idx_)) { // specific case for the vertical bars |
| sx = fabs(x1 - x2) / | ||
| 2; // rely on x precision being one single pad width |
There was a problem hiding this comment.
| sx = fabs(x1 - x2) / | |
| 2; // rely on x precision being one single pad width | |
| sx = fabs(x1 - x2) / 2; // rely on x precision being one single pad width |
There was a problem hiding this comment.
This I think was the clang tidy, the way out of it is to comment above the line
| sx = fabs(x1 - x2) / | |
| 2; // rely on x precision being one single pad width | |
| // rely on x precision being one single pad width | |
| sx = fabs(x1 - x2) / 2; |
bryngemark
added
the
trigscint
4 participants
I am updating ldmx-sw, here are the details.
As part of my bachelor thesis project, I have implemented a method of TS track-making using a lookup table to accommodate potential misalignment. To do this, a boolean variable "lut_tracking" has been added to TrigScint/src/TrigScint/TrigScintTrackProducer, so that this method and the (currently existing) method using max_delta can be 'switched between.' It reads a LUT file, a text file, in order to realize the new method; the LUT file is constructed using two new analyzers as described below. The LUT file is a list of cluster combinations from the three pads which appear "often enough" to be considered as originating from beam electrons.
I have added the following files:
I have changed the following files:
What are the issues that this addresses?
This resolves issue #2035.
Check List
- I successfully compiled ldmx-sw with my developments.
- I read, understood and follow the coding rules.
- I ran my developments and the following shows that they are successful.
Excerpt of runClusterstxt.py output, which utilizes ClusterTripletMaker.cxx to write the following clusters.txt file:
The columns here refer to event number, pad 1 cluster centroid, pad 2 cluster centroid, and pad 3 cluster centroid.
Excerpt of runLUTana.py output, which takes the runClusterstxt.py output and uses PatternLUTMaker.cxx to write the following LUT.txt:
The columns here refer to pad 1 cluster, pad 2 cluster, and pad 3 cluster for those rows in clusters.txt which pass the LUT threshold when grouped into patterns in PatternLUTMaker.
Here is track-making as performed in runLUTtracking.py for both lut-method tracking and current max_delta method tracking. The number of tracks made by lut-method tracking is dependent on the LUT threshold, here set at 0.0008:
For the max_delta definition tracking, as currently implemented in TrigScintTrackProducer:
For lut-method tracking, as proposed here (for a lut_threshold of 0.0008):
