Vertex Reconstruction Update #288

flemmons · 2024-10-02T02:22:01Z

Update to muon vertex reconstruction, to bring up to current work.
-Added current/updated toolchain configfiles PhaseIIRecoDir (PhaseIIRecoFirinne is a working version that comes with no guarantee), and PhaseIIRecoTruthDOE directories, due to some testing for comparison with old results; these are disorganized and can be rejected from the merge without issue; I'll fix them up for actual use for the next update.
-Updated DigitBuilder and EventSelector with check for LAPPD Multiplicity for vertex reconstruction purposes
-Updated DigitBuilder for reduction of LAPPD hits into strip-hits. Partial functionality; is a work in progress.
-Added DirectionGridCheck debug tool, which makes simple plots comparing various track-direction seeding methods
-Updated LikelihoodFitterCheck tool to make plots for multiple events in a single run
-Updated VertexGeometryCheck tool to show individual LAPPD hits, for purpose of debugging or analyzing in-depth effects of strip-hit reduction

…an time selection

Removed redundant GenerateFineGrid function in VtxSeedGenerator Removed experimental penalty term calculation in FoMCalculator

…r, and handling in VtxExtendedVertexFinder and MinuitOptimizer. This change is experimental, but should only trigger with appropriate config variables.

…l charge fit.

…y to vary 2D angle instead of position

…arge.root, which is the file containing pdf plot for use within new charge fitter.

Current temp

…rection fit

… of the old DOE MC files; (2) Updated EventSelectorDoE to avoid segmentation fault; (3) Updated the config files under PhaseIIRecoTruthDoE to run the reconstruction for the old DoE files

… May1

(2) Fixed a bug in Digitbuilder to avoid empty digit times

Added mulit-event functionality to LikelihoodFitterCheck

…emoval of new changes in last commit

marc1uk · 2024-12-17T22:42:13Z

UserTools/EventSelector/EventSelector.cpp

+      if (not has_LAPPDMult) {
+          Log("EventSelector Tool: No count of hit LAPPDs; proceeding without LAPPD multiplicity cut.", v_warning, verbosity);
+          fLAPPDMultMin = 0;
+          fHitLAPPDs = new std::vector<int>;


This Tool doesn't seem to call m_data->Stores.at("RecoEvent")->Set("HitLAPPDs",XXX), which means if HitLAPPDs doesn't exist for the first Execute call, it probably won't exist for any of them, and you'll be creating a new std::vector<int> on every loop, leaking them as you go.
Is there any reason this vector even needs to be on the heap? At the least please only new the vector if it doesn't exist.

marc1uk · 2024-12-17T22:43:03Z

UserTools/EventSelector/EventSelector.cpp

-  m_data->Stores.at("RecoEvent")->Set("RecoPDGVector",cluster_reco_pdg);
-  m_data->Stores.at("RecoEvent")->Set("PDG",fRecoPDG);
+  bool passRecoPDGCut = 0;
+  std::cout << "fRecoPDG: " << fRecoPDG << endl;


std::cout usage...

marc1uk · 2024-12-17T22:47:08Z

UserTools/EventSelector/EventSelector.cpp

+      m_data->Stores.at("RecoEvent")->Set("PDG", fRecoPDG);
+  }
+
+  bool passExtendedCut = this->EventSelectionByTriggerExtended();


nitpicking but this keeps coming up: there are only a very small number of obscure cases involving derived class pointers being dynamically cast to base class pointers where this-> is required. Just call the function:
passExtendedCut = EventSelectionByTriggerExtended();

marc1uk · 2024-12-17T23:01:05Z

UserTools/HitCleaner/HitCleaner.cpp

@@ -691,7 +693,7 @@ std::vector<RecoCluster*>* HitCleaner::RecoClusters(std::vector<RecoDigit*>* myD
    }
  }

-  //std::cout <<"fClusterList->size() = "<<fClusterList->size()<<std::endl;
+  std::cout <<"Number of clusters = "<<fClusterList->size()<<std::endl;


marc1uk · 2024-12-17T23:03:19Z

UserTools/LikelihoodFitterCheck/LikelihoodFitterCheck.cpp

+      if (!pdftest) {
+          Log("LikelihoodFitterCheck  Tool: Error retrieving pdffile; running without!", v_error, verbosity);
+          fUsePDFFile = 0;
+          return false;


should this return false? The printout suggests that it will continue without, but it doesn't. At least, not until the next Execute call.

marc1uk · 2024-12-17T23:06:17Z

UserTools/LikelihoodFitterCheck/LikelihoodFitterCheck.cpp

-    seedY = trueVtxY - 50*dy + j*dy;
-    seedZ = trueVtxZ - 50*dz + j*dz;
+  int nbins = 400;
+  double dlpara[400], dlfom[400];


is there any reason to accumulate these in an array? It seems like dlfom[j] is set to fom, and then the only time it's used is in the next line to access dlfom[j], where fom is still in scope. Just use fom? Same for dlpara.

Same remark for dltrans further down

marc1uk · 2024-12-17T23:14:31Z

UserTools/LikelihoodFitterCheck/LikelihoodFitterCheck.cpp

+          Log("VtxExtendedVertexFinder: pdffile does not exist", v_error, verbosity);
+          return false;
+      }
+      pdf = *(TH1D*)f1.Get("zenith");


would be nice to check if zenith exists, otherwise this will segfault.

marc1uk · 2024-12-17T23:17:31Z

UserTools/LoadWCSim/LoadWCSim.cpp

-	} // else if we're splitting subtriggers, loop from [current MCTriggernum] to [current MCTriggernum+1]
-
+	if (!splitSubtriggers) MCTriggernum = 0;
+	int MaxEventNr = (splitSubtriggers)? (MCTriggernum + 1) : (WCSimEntry->wcsimrootevent->GetNumberOfEvents());


is there a reason to have replaced this code with a version that removes the explanatory comments?

marc1uk · 2024-12-17T23:18:01Z

UserTools/LoadWCSim/LoadWCSim.cpp

@@ -631,18 +636,24 @@ bool LoadWCSim::Execute(){
 			} else {
 				// instead take the true time of the first photon
 				std::vector<int> photonids = digihit->GetPhotonIds();   // indices of the digit's photons
+				std::cout << "LoadWCSim Tool: number of photons in this hit: " << photonids.size() << endl;


std::cout usage

marc1uk · 2024-12-17T23:38:17Z

UserTools/LoadWCSim/LoadWCSim.cpp

+					digittime = earliestphotontruetime;
+				}
+				else {
+


...? Is there supposed to be some alternative here? Otherwise if firstHit is false, we just leave the digit times uninitialised. 🤨

marc1uk · 2024-12-17T23:41:15Z

UserTools/LoadWCSim/LoadWCSim.cpp

-	// so downstream tools know it's a new event
-	int reported_triggernum = splitSubtriggers ? MCTriggernum-1 : 0;
-	m_data->Stores.at("ANNIEEvent")->Set("MCTriggernum",reported_triggernum);
+	m_data->Stores.at("ANNIEEvent")->Set("MCTriggernum",MCTriggernum-1);


has this been validated? Why was this change made?

marc1uk · 2024-12-17T23:44:13Z

UserTools/LoadWCSim/wcsimT.h

@@ -47,7 +47,7 @@ public :
   WCSimRootOptions   *wcsimrootopts=nullptr;

   // TODO implement a verbosity arg
-   int             verbose=1;
+   int             verbose=5;


there's no point changing this as both constructors will set the verbosity, so this value will always be overwritten.

it's also bad practice to set default verbosities so high. Generally they should set to only print errors and warnings.

feels like none of the changes in this file should be committed....

marc1uk · 2024-12-17T23:47:38Z

UserTools/MeanTimeCheck/MeanTimeCheck.cpp

+	Log("MeanTimeCheck Tool: Executing", v_debug, verbosity);
+	auto* reco_event = m_data->Stores["RecoEvent"];
+	if (!reco_event) {
+		Log("Error: The MeanTimeCheck tool could not find the ANNIEEvent Store", 0, verbosity);


again name of store doesn't tie up with the error message, and this isn't a good way to do this. Same comments as DirectionGridCheck.

marc1uk · 2024-12-17T23:59:17Z

UserTools/MeanTimeCheck/MeanTimeCheck.cpp

+	double timeMin = deltaTime1.at(int((n - 1)*0.05)); // 5% of the total entries
+	double timeMax = deltaTime1.at(int((n - 1)*0.90)); // 90% of the total entries
+	int nbins = int(n / 5);
+	TH1D *hDeltaTime = new TH1D("hDeltaTime", "hDeltaTime", nbins, timeMin, timeMax);


why does this need to be on the heap? heap allocation is slow and risks memory leaks, this could just as easily be declared on the stack with no such issues.

marc1uk · 2024-12-18T00:10:07Z

UserTools/VertexGeometryCheck/VertexGeometryCheck.cpp

  return true;
 }

 bool VertexGeometryCheck::Finalise(){
  fOutput_tfile->cd();
  fOutput_tfile->Write();
+  if (StripTimePlot > 0) StripHits1->Write("lappdtimegradient");


this name doesn't seem consistent with the usage in the above loop. Is this correct?

marc1uk · 2024-12-18T00:12:28Z

VGCheck-pdfcharge.root

In general we don't add binary files to source code repositories. Please instead make the path to this file a configuration variable if it isn't already, and put a copy of this file somewhere appropriate on /pnfs/annie/persistent, with the default value of the configured path directed there.

marc1uk · 2024-12-18T00:17:15Z