Per Plane Hit Tuning Parameters for GaussHitFinderSBN by carriganm95 · Pull Request #626 · SBNSoftware/sbncode

carriganm95 · 2026-02-04T21:15:31Z

Description

This PR adds per plane variables for the GaussHitFinderSBN chi2 and maxmultihit parameters. It also modifies some parameters to be those found to perform the best from a grid search over a parameter space.

Copilot

Pull request overview

This PR introduces per-plane tuning parameters for the GaussHitFinderSBN module, allowing different chi-squared and maximum multi-hit thresholds for each detector plane. The parameter values have been optimized through a grid search to improve hit finding performance.

Changes:

Added per-plane MaxMultiHitPerPlane and Chi2NDFPerPlane parameters to replace single-value MaxMultiHit and Chi2NDF parameters
Updated LongPulseWidth from [10, 10, 10] to [5, 5, 5] and MinPulseHeight from [5.0, 5.0, 5.0] to [2.0, 2.0, 2.0] based on grid search optimization
Added sbnanaobj StandardRecord dependencies to CAFMaker/RecoUtils CMakeLists.txt

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
sbncode/HitFinder/hitfindermodules_sbn.fcl	Adds per-plane parameter arrays and updates tuned parameter values; retains old deprecated parameters
sbncode/HitFinder/GaussHitFinderSBN_module.cc	Converts MaxMultiHit and Chi2NDF from scalars to vectors and updates all usages with plane indexing
sbncode/CAFMaker/RecoUtils/CMakeLists.txt	Adds StandardRecord dependencies (appears unrelated to hit finder changes)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T21:20:54Z

sbncode/HitFinder/GaussHitFinderSBN_module.cc

    , fAreaMethod(pset.get<int>("AreaMethod"))
    , fAreaNormsVec(FillOutHitParameterVector(pset.get<std::vector<double>>("AreaNorms")))
-    , fChi2NDF(pset.get<double>("Chi2NDF"))
+    , fChi2NDF(pset.get<std::vector<double>>("Chi2NDFPerPlane", std::vector<double>() = {500.0, 500.0, 500.0}))


The Chi2NDFPerPlane parameter is not validated to ensure its size matches the number of planes in the geometry. Other similar parameters like AreaNorms use FillOutHitParameterVector to validate size and handle cases where the input vector has 1 element or N_PLANES elements. Without this validation, if a user provides a vector with an incorrect size, the code could fail at runtime with an out-of-bounds error when accessing fChi2NDF.at(plane). Consider using FillOutHitParameterVector or adding similar validation to ensure the vector size matches the geometry.

Suggested change

, fChi2NDF(pset.get<std::vector<double>>("Chi2NDFPerPlane", std::vector<double>() = {500.0, 500.0, 500.0}))

, fChi2NDF(FillOutHitParameterVector(

pset.get<std::vector<double>>("Chi2NDFPerPlane",

std::vector<double>() = {500.0, 500.0, 500.0})))

Copilot · 2026-02-04T21:20:54Z

sbncode/HitFinder/GaussHitFinderSBN_module.cc

    const std::vector<double>
      fAreaNormsVec;       ///<factors for converting area to same units as peak height
-    const double fChi2NDF; ///maximum Chisquared / NDF allowed for a hit to be saved
+    const std::vector<double> fChi2NDF; ///maximum Chisquared / NDF allowed for a hit to be saved


Comment formatting is inconsistent with surrounding code. The comment should use three slashes '///<' to match the style used for other member variable comments in this class (e.g., line 82).

Suggested change

const std::vector<double> fChi2NDF; ///maximum Chisquared / NDF allowed for a hit to be saved

const std::vector<double> fChi2NDF; ///<maximum Chisquared / NDF allowed for a hit to be saved

Copilot · 2026-02-04T21:20:54Z

sbncode/HitFinder/hitfindermodules_sbn.fcl

-    AllHitsInstanceName:  ""                 # If non-null then this will be the instance name of all hits output to event
-                                             # in this case there will be two hit collections, one filtered and one containing all hits
+    MaxMultiHit:          5                       # maximum hits for multi gaussian fit attempt
+    MaxMultiHitPerPlane:  [ 10, 10, 8 ]           # maximum hits per plane for multi gaussia fit attempt


Spelling error in comment: "gaussia" should be "gaussian".

Suggested change

MaxMultiHitPerPlane: [ 10, 10, 8 ] # maximum hits per plane for multi gaussia fit attempt

MaxMultiHitPerPlane: [ 10, 10, 8 ] # maximum hits per plane for multi gaussian fit attempt

Copilot · 2026-02-04T21:20:55Z

sbncode/HitFinder/hitfindermodules_sbn.fcl

-                                             # will use "long" pulse method to return hit
-    AllHitsInstanceName:  ""                 # If non-null then this will be the instance name of all hits output to event
-                                             # in this case there will be two hit collections, one filtered and one containing all hits
+    MaxMultiHit:          5                       # maximum hits for multi gaussian fit attempt


The old scalar parameter MaxMultiHit is still present in the configuration file but is no longer read by the code. The code now only reads MaxMultiHitPerPlane. This parameter should be removed to avoid confusion and maintain consistency, as it no longer has any effect on the module's behavior.

Suggested change

MaxMultiHit: 5 # maximum hits for multi gaussian fit attempt

Copilot · 2026-02-04T21:20:55Z

sbncode/HitFinder/hitfindermodules_sbn.fcl

+    TryNplus1Fits:        false                   # Don't try to refit with extra peak if bad chisq
+    LongMaxHits:          [ 25, 25, 25]           # max number hits in long pulse trains
+    LongPulseWidth:       [ 5, 5, 5]              # max widths for hits in long pulse trains
+    Chi2NDF:              500.                    # maximum Chisquared / NDF allowed to store fit


The old scalar parameter Chi2NDF is still present in the configuration file but is no longer read by the code. The code now only reads Chi2NDFPerPlane. This parameter should be removed to avoid confusion and maintain consistency, as it no longer has any effect on the module's behavior.

Suggested change

Chi2NDF: 500. # maximum Chisquared / NDF allowed to store fit

Copilot · 2026-02-04T21:20:55Z

sbncode/CAFMaker/RecoUtils/CMakeLists.txt

+                    sbnanaobj::StandardRecord
+                    sbnanaobj::StandardRecordFlat


The CMakeLists.txt changes add sbnanaobj StandardRecord dependencies, but these don't appear to be used by RecoUtils.cc or RecoUtils.h. Neither file includes or references StandardRecord or StandardRecordFlat. This change appears unrelated to the hit finder tuning described in the PR description. If these dependencies are needed for other purposes, that should be explained. If not, they should be removed as they add unnecessary dependencies and build-time coupling.

From the header included in RecoUtils.{h,cpp} it looks like this dependency is still not needed. I request that you remove that and verify that the code still builds.

Copilot · 2026-02-04T21:20:55Z

sbncode/HitFinder/GaussHitFinderSBN_module.cc

    , fLongPulseWidthVec(
        pset.get<std::vector<int>>("LongPulseWidth", std::vector<int>() = {16, 16, 16}))
-    , fMaxMultiHit(pset.get<int>("MaxMultiHit"))
+    , fMaxMultiHit(pset.get<std::vector<size_t>>("MaxMultiHitPerPlane", std::vector<size_t>() = {5, 5, 5}))


The MaxMultiHitPerPlane parameter is not validated to ensure its size matches the number of planes in the geometry. Other similar parameters like AreaNorms use FillOutHitParameterVector to validate size and handle cases where the input vector has 1 element or N_PLANES elements. Without this validation, if a user provides a vector with an incorrect size, the code could fail at runtime with an out-of-bounds error when accessing fMaxMultiHit.at(plane). Consider using FillOutHitParameterVector or adding similar validation to ensure the vector size matches the geometry.

kjplows · 2026-02-11T15:43:05Z

Summoning @PetrilloAtWork and @henrylay97 for review, could you please take a look at this? Thanks!

henrylay97

Looks like copilot got in there with most of the good comments ;)

I have one concern about preserving default behaviour for SBND.

henrylay97 · 2026-02-12T09:25:54Z

sbncode/HitFinder/hitfindermodules_sbn.fcl

-    AllHitsInstanceName:  ""                 # If non-null then this will be the instance name of all hits output to event
-                                             # in this case there will be two hit collections, one filtered and one containing all hits
+    MaxMultiHit:          5                       # maximum hits for multi gaussian fit attempt
+    MaxMultiHitPerPlane:  [ 10, 10, 8 ]           # maximum hits per plane for multi gaussia fit attempt


The default behaviour should be retained here as this will be inherited by SBND as well. i.e. [5, 5, 5].

You will then want to override this parameter in an ICARUS fcl.

henrylay97 · 2026-02-12T09:26:23Z

sbncode/HitFinder/hitfindermodules_sbn.fcl

+    LongMaxHits:          [ 25, 25, 25]           # max number hits in long pulse trains
+    LongPulseWidth:       [ 5, 5, 5]              # max widths for hits in long pulse trains
+    Chi2NDF:              500.                    # maximum Chisquared / NDF allowed to store fit
+    Chi2NDFPerPlane:      [ 2500., 1750., 2500.]  # maximum Chisquared / NDF allowed per plane to store, if fail


Same comment here, need to retain default behaviour.

mattiasotgia · 2026-02-28T01:02:34Z

Hi! I am taking over from Michael's work on the hit finder tuning and thought of chiming in replying to Henry's point.

I agree that even for the ICARUS reconstruction this might be a breaking change (we would need to update all the relative FHiCLs), and would suggest a way of going about this.

We should keep only the Chi2NDF and MaxMultiHit parameters, and have both them being "mutable" and be either a single value or a tuple (one per plane) as desired.

The changes would still require the private attributes to be changed to vectors

const std::vector<double> fChi2NDF; 
const std::vector<size_t> fMaxMultiHit;

With the introduction of the utility function (suggested by @PetrilloAtWork)

std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) {
  auto const& wireReadoutGeom = art::ServiceHandle<geo::WireReadout const>()->Get();
  const unsigned int N_PLANES = wireReadoutGeom.Nplanes();
  if (pset.is_key_to_sequence(key))
    return pset.get<std::vector<T>>(key);
  else
    return std::vector<T>(N_PLANES, pset.get<T>(key));
} // getValueOrListOf()

the interface to assign them would change to

GaussHitFinderSBN::GaussHitFinderSBN(fhicl::ParameterSet const& pset, art::ProcessingFrame const&)
  : SharedProducer{pset},
    ...
    fChi2NDF(getValueOrListOf<double>(pset, "Chi2NDF")),
    fMaxMultiHit(getValueOrListOf<size_t>(pset, "MaxMultiHit")),
    ...

PetrilloAtWork · 2026-02-28T01:18:37Z

Another, less elegant, possibility is to read the object as a sequence (which if I don't get it wrong is what was happening) and to extend it to NPlanes with the last specified value if necessary (so, [ 5 ] and [ 5, 5 ] would turn into std::vector<int>{ 5, 5, 5 }).
I am not advocating for this idea, but it's there.

…ne and Chi2PerPlane

carriganm95 · 2026-03-04T21:08:14Z

Hi! I am taking over from Michael's work on the hit finder tuning and thought of chiming in replying to Henry's point.

I agree that even for the ICARUS reconstruction this might be a breaking change (we would need to update all the relative FHiCLs), and would suggest a way of going about this.

We should keep only the Chi2NDF and MaxMultiHit parameters, and have both them being "mutable" and be either a single value or a tuple (one per plane) as desired.

The changes would still require the private attributes to be changed to vectors
const std::vector<double> fChi2NDF; 
const std::vector<size_t> fMaxMultiHit;
With the introduction of the utility function (suggested by @PetrilloAtWork)
std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) {
  auto const& wireReadoutGeom = art::ServiceHandle<geo::WireReadout const>()->Get();
  const unsigned int N_PLANES = wireReadoutGeom.Nplanes();
  if (pset.is_key_to_sequence(key))
    return pset.get<std::vector<T>>(key);
  else
    return std::vector<T>(N_PLANES, pset.get<T>(key));
} // getValueOrListOf()
the interface to assign them would change to
GaussHitFinderSBN::GaussHitFinderSBN(fhicl::ParameterSet const& pset, art::ProcessingFrame const&)
  : SharedProducer{pset},
    ...
    fChi2NDF(getValueOrListOf<double>(pset, "Chi2NDF")),
    fMaxMultiHit(getValueOrListOf<size_t>(pset, "MaxMultiHit")),
    ...

These changes have been implemented thanks to @mattiasotgia !

PetrilloAtWork

I left two technical comments and support for one of CoPilot comments (the others may be outdated).
In particular, CoPilot comment needs to be addressed.

PetrilloAtWork · 2026-03-23T18:57:37Z

sbncode/HitFinder/GaussHitFinderSBN_module.cc

    std::vector<double> FillOutHitParameterVector(const std::vector<double>& input);
+
+    template<class T>
+    inline std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) {


The inline keyword is redundant in case of:

template definitions (which must always be treated in a inline-like way);

definitions of member functions inside the definition of the class (where inline is implicit).

Consider removing it. On the other end, this function does not need to change the state of the class it belongs to, so it should be declared const. In fact, since this is a general utility that does not depend on the class at all, it could even be declared static.
I recommend one of the two changes, and suggest the former (which allows future caching of the number of planes):

Suggested change

inline std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) {

std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) const {

PetrilloAtWork · 2026-03-23T19:07:49Z

sbncode/HitFinder/GaussHitFinderSBN_module.cc

+      auto const& wireReadoutGeom = art::ServiceHandle<geo::WireReadout const>()->Get();
+      const unsigned int N_PLANES = wireReadoutGeom.Nplanes();
+
+      if (pset.is_key_to_sequence(key))
+        return pset.get<std::vector<T>>(key);
+      else
+        return std::vector<T>(N_PLANES, pset.get<T>(key));


Consider to enforce that in case users specified a list of numbers, that is of the right size. If you don't want to do that, or not want to do that here, then move the discovery of the number of planes in the if branch where it is needed:

Suggested change

auto const& wireReadoutGeom = art::ServiceHandle<geo::WireReadout const>()->Get();

const unsigned int N_PLANES = wireReadoutGeom.Nplanes();

if (pset.is_key_to_sequence(key))

return pset.get<std::vector<T>>(key);

else

return std::vector<T>(N_PLANES, pset.get<T>(key));

if (pset.is_key_to_sequence(key))

return pset.get<std::vector<T>>(key);

else {

auto const& wireReadoutGeom = art::ServiceHandle<geo::WireReadout const>()->Get();

const unsigned int N_PLANES = wireReadoutGeom.Nplanes();

return std::vector<T>(N_PLANES, pset.get<T>(key));

}

so that the service is not fetched unless it is strictly required.

PetrilloAtWork · 2026-03-23T19:20:50Z

sbncode/CAFMaker/RecoUtils/CMakeLists.txt

+                    sbnanaobj::StandardRecord
+                    sbnanaobj::StandardRecordFlat


From the header included in RecoUtils.{h,cpp} it looks like this dependency is still not needed. I request that you remove that and verify that the code still builds.

update GaussHitFinderSBN_module to take per plane hit finding parameters

805020e

Copilot AI review requested due to automatic review settings February 4, 2026 21:15

Copilot started reviewing on behalf of carriganm95 February 4, 2026 21:15 View session

carriganm95 marked this pull request as draft February 4, 2026 21:15

Copilot AI reviewed Feb 4, 2026

View reviewed changes

carriganm95 changed the title ~~Draft: Per Plane Hit Tuning Parameters for GaussHitFinderSBN~~ Per Plane Hit Tuning Parameters for GaussHitFinderSBN Feb 4, 2026

carriganm95 marked this pull request as ready for review February 4, 2026 21:29

kjplows assigned carriganm95 Feb 11, 2026

kjplows requested review from PetrilloAtWork and henrylay97 February 11, 2026 15:43

kjplows added this to SBN software development and PR archaeology Feb 11, 2026

github-project-automation bot moved this to Todo in PR archaeology Feb 11, 2026

kjplows moved this to Open pull requests in SBN software development Feb 11, 2026

kjplows moved this from Todo to In Progress in PR archaeology Feb 11, 2026

henrylay97 reviewed Feb 12, 2026

View reviewed changes

Updates from Mattia to allow only one definition of MaxMultiHitPerPla…

5977b29

…ne and Chi2PerPlane

PetrilloAtWork requested changes Mar 23, 2026

View reviewed changes

	const std::vector<double> fChi2NDF; ///maximum Chisquared / NDF allowed for a hit to be saved
	const std::vector<double> fChi2NDF; ///<maximum Chisquared / NDF allowed for a hit to be saved

	MaxMultiHitPerPlane: [ 10, 10, 8 ] # maximum hits per plane for multi gaussia fit attempt
	MaxMultiHitPerPlane: [ 10, 10, 8 ] # maximum hits per plane for multi gaussian fit attempt

	inline std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) {
	std::vector<T> getValueOrListOf(fhicl::ParameterSet const& pset, std::string const& key) const {

Conversation

carriganm95 commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

kjplows commented Feb 11, 2026

Uh oh!

henrylay97 left a comment

Choose a reason for hiding this comment

Uh oh!

henrylay97 Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

henrylay97 Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

mattiasotgia commented Feb 28, 2026

Uh oh!

PetrilloAtWork commented Feb 28, 2026

Uh oh!

carriganm95 commented Mar 4, 2026

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carriganm95 commented Feb 4, 2026 •

edited

Loading