ClusterSearcher and RecoCluster #365

flemmons · 2025-10-27T16:02:25Z

-Updated DigitBuilder to be more consistent with and independent of ClusterFinder's corresponding functions
--Added some extra features, such as hit LAPPD count and strip-by-strip LAPPD hits. (some of these are considered experimental/preliminary)
-Added ClusterSearcher to replace ClusterFinder using RecoDigit and RecoCluster classes
-Updated RecoDigit and RecoCluster classes for corresponding use and new features, such as various cluster parameters
-Added NeutronCheck tool as output for RecoCluster information
-Added sample toolchain configfolder for using the new tools

…ClusterFinder's corresponding functions --Added some extra features, such as hit LAPPD count and strip-by-strip LAPPD hits. -Added ClusterSearcher to replace ClusterFinder using RecoDigit and RecoCluster classes -Updated RecoDigit and RecoCluster classes for corresponding use and new features, such as various cluster parameters -Added NeutronCheck tool as output for RecoCluster information -Added sample toolchain configfolder for using the new tools

jminock · 2025-11-06T15:02:26Z

UserTools/DigitBuilder/DigitBuilder.cpp

+    }
+
+
+    /*while (!file_singlepe.eof()) {


Please delete the old commented out code. It unnecessarily clutters the file.

jminock · 2025-11-06T15:10:07Z

DataModel/RecoCluster.cpp

+            //}
+        }
+    }
+    //FIXME: Need a method to have the 123 be equal to the number of operating detectors


Should this be fixed now?

jminock · 2025-11-06T15:17:47Z

DataModel/RecoCluster.cpp

+        double max_angle = 0;
+        double angle;
+        Position i_position, j_position;
+        for (int i = 0; i < fDigitList.size(); i++) {


There are many separate loops of the same size with the same index going on. Could we combine them? If this is a DataModel function, it could be used across multiple Tools so it would be best to improve efficiency within reason.

S81D · 2025-11-12T17:32:16Z

configfiles/ClusterSearcher/LoadWCSimConfig

+LappdNumStrips 60            ## num channels to construct from each LAPPD
+LappdStripLength 100         ## relative x position of each LAPPD strip, for dual-sided readout [mm]
+LappdStripSeparation 10      ## stripline separation, for calculating relative y position of each LAPPD strip [mm]
+PMTMask configfiles/BeamClusterAnalysisMC/DeadPMTIDs_p2v7.txt ## Which PMTs should be masked out? / are dead?


You can point to the most up to date path configfiles/LoadWCSim/DeadPMTIDs_p2v7.txt

S81D · 2025-11-12T17:52:54Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+    if (!fisMC){
+      int pmtid = recoDigit->GetDetectorID();
+      unsigned long chankey = pmt_tubeid_to_channelkey[pmtid];
+      if (pmt_gains[chankey]>0) qep/=pmt_gains[chankey];


In line 77 (m_variables.Get("SinglePEGains",singlePEgains);) you grab the SPE gains from the gains file then use it to convert from nQ --> pe for data. It doesn't look like you ever specify the gains file in the Config file. Could you add it to ClusterSearcherConfig? Alternatively you can grab that directly from the Store since the LoadGeometry tool populates it for downstream tools (like this one) to use. See PhaseIITreeMaker for an example.

The current configuration in the provided toolchain is for use on MC simulations, which do not require the conversion. So this variable is not necessary within the configuration. I will add it to the readme file.
Though as I start looking at Data in the next couple of weeks, I'll likely move into taking the gains from the store in the next update to this tool. Thank you for the suggestion.

S81D · 2025-11-12T17:55:30Z

UserTools/NeutronCheck/NeutronCheck.cpp

+            if(fMCParticles->at(i).GetPdgCode()==2112 && fMCParticles->at(i).GetParentPdg()==0) {
+                fTrueNeutronMult++;
+
+                if(fMCParticles->at(i).GetStopTime()>10000) fTrueNeutronDelayed++;


It might be wise to make this a configurable variable in case someone needs to look for neutrons in a different region of interest. Especially considering 10us is used extensively throughout the code.

S81D · 2025-11-12T17:56:58Z

UserTools/NeutronCheck/NeutronCheck.cpp

+        //true_Emu*=1000;  //GeV->MeV to match other energies(unneeded, possibly)
+
+        double theta = truevtx->GetDirection().GetTheta();
+        double p = sqrt(pow(true_Emu,2)-pow(105.7,2));


@jminock is there a way to grab the momentum and Q^2 from the Store? I thought the LoadGenieEvent tool will store those values for use.

There absolutely is via: m_data->Stores["GenieInfo"]->Get("EventQ2",TrueQ2). Magnitude of muon momentum is not saved to the Store so this is the intended method to get the true muon momentum

jminock

Please make the changes listed. I worry with the large number of pointers if memory is being handled properly. I don't know how necessary all of the pointers are, and I am not enough of an expert on it to make a certain statement. I recommend double checking all are fitting general best practice before this gets sent off to Level 0 review

jminock · 2025-11-13T02:35:35Z

DataModel/RecoCluster.cpp

+    double maxCharge=0;
+    int tempPDG = -5;
+    for (RecoDigit* i_digit : fDigitList) {
+        cout<<"Digit parent list size: "<<i_digit->GetParents().size()<<endl;


Please add verbosity conditions to the cout's throughout this file and others. Preferably Log functions if you would like Marcus to give approval down the road

jminock · 2025-11-13T02:44:46Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+
+  // Default Clustering parameters
+  fConfig = ClusterSearcher::kPulseHeightAndClusters;
+  fPmtMinPulseHeight = 5.0;     // minimum pulse height (PEs) //Ioana... initial 1.0


The configuration file should take care of the initialization. Leave the default parameters in the configuration file. If any of these NEED to be initialized outside of the configuration file, please do so in the header file upon declaration. It cuts down on clutter

jminock · 2025-11-13T02:46:26Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+    double temp_gain;
+    while (!file_singlepe.eof()){
+      file_singlepe >> temp_chankey >> temp_gain;
+      if (file_singlepe.eof()) break;


This line seems unnecessary. While loop already includes this condition

i think it would also prevent the emplace call for the last line of the file.

jminock · 2025-11-13T02:51:49Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+
+      carryon = 1;
+      while( carryon ){
+        carryon = 0;


Why is this for loop inside of a while loop?

Seemingly because only one of the two loops had a well-defined range over which to run. This code was inherited from the HitCleaner tool, so I can't answer the why of that decision with complete certainty, but the code flows to my reading, and works as needed.
This seems like a pet peeve, more than a problem, so for the time being, I'll stand by it as is.

jminock · 2025-11-13T02:54:19Z

UserTools/DigitBuilder/DigitBuilder.cpp


  /////////////////// Usefull header ///////////////////////
-  if(verbosity) cout<<"Initializing Tool DigitBuilder"<<endl;
+  cout<<"Initializing Tool DigitBuilder"<<endl;


I think this has been mentioned before. Please include verbosity conditions, preferably as Log functions

DataModel/RecoCluster.cpp

marc1uk · 2025-11-12T23:30:42Z

DataModel/RecoCluster.cpp

+    double sumY=0;
+    Position pos;
+    //std::vector<RecoDigits> hullDigits;
+    std::sort(fDigitList.begin(), fDigitList.end());


is sorting a vector of pointers with the default comparator going to do what you expect? I see you have a operator< for RecoDigit, but think you still need to provide a comparitor lambda to perform the pointer de-reference:

std::sort(fDigitList.begin(), fDigitList.end(), [](const RecoDigit* a, const RecoDigit* b){ return a < b; });

I'm... just going to remove the RecoCluster::ConvexHull function, at least for now. It's not actually useful right now.

marc1uk · 2025-11-13T12:23:51Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+  }
+
+  if( !fgClusterSearcher ){
+    assert(fgClusterSearcher);


this seems redundant. If the previous allocation didn't succeed it would have thrown bad_alloc.

marc1uk · 2025-11-13T12:24:31Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+  }
+
+  if( fgClusterSearcher ){
+


marc1uk · 2025-11-13T12:32:47Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+  }
+
+  if (!fisMC){
+    ifstream file_singlepe(singlePEgains.c_str());


recommend adding a check file_singlepe.is_open() to catch typos in filename.

marc1uk · 2025-11-13T17:29:33Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+
+  // run clustering algorithm
+  // ========================
+  std::vector<RecoCluster*>* ClusterList = (std::vector<RecoCluster*>*)(this->RecoClusters(DigitList));


again shouldn't need the c-style cast - in fact the local variable ClusterList is just an alias for the member variable fRecoClusters, so seems redundant. The aliasing in this Tool makes it hard to track objects.

marc1uk · 2025-11-13T17:31:04Z

UserTools/ClusterSearcher/ClusterSearcher.cpp

+  // ===================
+  for(int idigit=0; idigit<int(fSelectByNeighbours->size()); idigit++ ){
+    RecoDigit* recoDigit = (RecoDigit*)(fSelectByNeighbours->at(idigit));
+    RecoClusterDigit* clusterDigit = new RecoClusterDigit(recoDigit);


why do the digits need to be on the heap?

marc1uk · 2025-11-13T18:45:56Z

UserTools/DigitBuilder/DigitBuilder.cpp

+        //Loop over lines, collect all detector data (should only be one line here)
+        while (getline(file_singlepe, line)) {
+            if (verbosity > 3) std::cout << line << std::endl; //has our stuff;
+            if (line.find("#") != std::string::npos) continue;


be wary that this will skip lines with trailing comments, as well as commented-out lines.
perhaps if(line.empty() || line[0]=='#') continue is a safer/clearer check

marc1uk · 2025-11-13T18:48:36Z

UserTools/DigitBuilder/DigitBuilder.cpp

+            if (verbosity > 3) std::cout << line << std::endl; //has our stuff;
+            if (line.find("#") != std::string::npos) continue;
+            std::vector<std::string> DataEntries;
+            boost::split(DataEntries, line, boost::is_any_of(","), boost::token_compress_on);


i think token_compress_on merges repeated tokens? Is this desirable? If there are repeated tokens, doesn't this suggest an empty column, and by compressing that, your later columns will be shifted?

marc1uk · 2025-11-13T18:50:18Z

UserTools/DigitBuilder/DigitBuilder.cpp

+              Log("This HIT'S TIME AND CHARGE: " + to_string(ahit.GetTime()) + ", " + to_string(ahit.GetCharge()),v_debug,verbosity);
            double hitTime = ahit.GetTime()*1.0;
-          	if(hitTime>-10 && hitTime<40) {
+          	if(hitTime>-10 && hitTime<70) {


better for magic numbers to be configuration variables, especially if they are subject to change

jminock self-requested a review October 30, 2025 13:03

jminock self-assigned this Oct 30, 2025

S81D self-assigned this Oct 30, 2025

jminock reviewed Nov 6, 2025

View reviewed changes

S81D reviewed Nov 12, 2025

View reviewed changes

jminock suggested changes Nov 13, 2025

View reviewed changes

marc1uk reviewed Nov 13, 2025

View reviewed changes

jminock added the waiting for submitter label Dec 4, 2025

ClusterSearcher and RecoCluster #365

Are you sure you want to change the base?

ClusterSearcher and RecoCluster #365

Uh oh!

Conversation

flemmons commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S81D Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jminock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc1uk Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

S81D Nov 12, 2025 •

edited

Loading

marc1uk Nov 13, 2025 •

edited

Loading