Controller output features and integration fixes by austinschneider · Pull Request #112 · Harvard-Neutrino/SIREN

austinschneider · 2026-04-28T04:29:14Z

Stack: PR 14 of 14

This PR is part of a 14-PR stack decomposing dev/HNL_DIS into reviewable chunks.

Base: feat/hnl-3body-sampling
Head: feat/controller-output

Merge order: feat/hnl-3body-sampling should land before this PR.

Squashed contents

Squashed diff for paths:

python/SIREN_Controller.py
projects/injection/private/Weighter.cxx
projects/injection/public/SIREN/injection/Weighter.h
projects/injection/private/pybindings/injection.cxx
cmake/Packages/CFITSIO.cmake
vendor/googletest

Source commits on dev/HNL_DIS_clean that contributed:
088b1cd (Nicholas Kamp): move around the HNL DIS fits files
0a9c9aa (Nicholas Kamp): ability to save interaction probabilities for each process
0c7ad93 (Nicholas Kamp): support sphere volume distributions in python interface
197bf14 (Nicholas Kamp): fixing build errors from the merge
1d68f30 (Nicholas Kamp): fix runtime errors on HNL generation related to particle types
4f113a1 (Nicholas Kamp): add survival probability function
521695f (Nicholas Kamp): fix issues with 2D tabular integral calculation, sampling errors with very short decay lengths, DarkNews errors, and kinematic erros in Dipole portal HNL DIS
631c12a (Nicholas Kamp): add ability to save interaction parameters to output file
82bcb80 (Nicholas Kamp): fix error in loading mat/det files
ac0ce24 (Nicholas Kamp): changes to facilitate hadronic and W/Z HNL decays
cf24f42 (Nicholas Kamp): add secondary fid vol flag in controller
da5f6b2 (Nicholas Kamp): detector changes, functionality to save initial position of interaction_record, and fix to HNLDISFromSpline.cxx
f183191 (Nicholas Kamp): more complex logic for adding darknews interactions, fix some errors in SIREN_DarkNews

Notes

This branch is the squashed cumulative diff of the listed source commits. Per-commit history is browsable on dev/HNL_DIS_clean.
Large .fits data files have been removed from the branch and are packaged separately.

Squashed diff for paths: - python/SIREN_Controller.py - projects/injection/private/Weighter.cxx - projects/injection/public/SIREN/injection/Weighter.h - projects/injection/private/pybindings/injection.cxx - cmake/Packages/CFITSIO.cmake - vendor/googletest Source commits on dev/HNL_DIS_clean that contributed: 088b1cd (Nicholas Kamp): move around the HNL DIS fits files 0a9c9aa (Nicholas Kamp): ability to save interaction probabilities for each process 0c7ad93 (Nicholas Kamp): support sphere volume distributions in python interface 197bf14 (Nicholas Kamp): fixing build errors from the merge 1d68f30 (Nicholas Kamp): fix runtime errors on HNL generation related to particle types 4f113a1 (Nicholas Kamp): add survival probability function 521695f (Nicholas Kamp): fix issues with 2D tabular integral calculation, sampling errors with very short decay lengths, DarkNews errors, and kinematic erros in Dipole portal HNL DIS 631c12a (Nicholas Kamp): add ability to save interaction parameters to output file 82bcb80 (Nicholas Kamp): fix error in loading mat/det files ac0ce24 (Nicholas Kamp): changes to facilitate hadronic and W/Z HNL decays cf24f42 (Nicholas Kamp): add secondary fid vol flag in controller da5f6b2 (Nicholas Kamp): detector changes, functionality to save initial position of interaction_record, and fix to HNLDISFromSpline.cxx f183191 (Nicholas Kamp): more complex logic for adding darknews interactions, fix some errors in SIREN_DarkNews

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds controller-side output toggles and new weighter APIs to expose interaction/survival probabilities, alongside integration/build fixes for model-file handling and CFITSIO discovery.

Changes:

Extend SIREN_Controller to support explicit detector/material model paths and to optionally save interaction probabilities/parameters/survival probabilities.
Add Weighter C++ APIs (and pybindings) to retrieve per-interaction probabilities and survival probabilities.
Improve CFITSIO CMake discovery by searching CMAKE_PREFIX_PATH.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
python/SIREN_Controller.py	Adds new controller options for model files, fiducial restrictions for secondaries, and richer event output fields.
projects/injection/public/SIREN/injection/Weighter.h	Declares new probability accessors on `ProcessWeighter`/`Weighter`.
projects/injection/private/Weighter.cxx	Implements per-interaction probability and survival probability extraction.
projects/injection/private/pybindings/injection.cxx	Exposes the new `Weighter` APIs to Python via pybind11.
cmake/Packages/CFITSIO.cmake	Extends CFITSIO search paths to include `CMAKE_PREFIX_PATH`.
vendor/googletest	Vendored dependency updated (not shown in diff excerpt).

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

Copilot · 2026-04-28T04:37:03Z

+            print("Must provide either an experiment name or both a detector model file and materials model file. Exiting")
+            exit(0)


exit(0) reports success even though initialization failed, which can silently mask errors in calling code and CI. Prefer raising a Python exception (e.g., ValueError) for library code; if this is truly CLI-only behavior, use a non-zero exit code (e.g., sys.exit(1)).

Suggested change

print("Must provide either an experiment name or both a detector model file and materials model file. Exiting")

exit(0)

raise ValueError("Must provide either an experiment name or both a detector model file and materials model file.")

Copilot · 2026-04-28T04:37:03Z

+                if self.primary_physical_process.primary_type == _dataclasses.Particle.ParticleType.unknown:
+                    self.primary_physical_process.primary_type = primary_interaction_collection.GetPrimaryType()
+                else:
+                    assert(self.primary_injection_process.primary_type == primary_interaction_collection.GetPrimaryType())


The assertion in the physical branch compares against self.primary_injection_process.primary_type rather than self.primary_physical_process.primary_type. This can incorrectly pass/fail depending on prior injection configuration and can hide a mismatch in the physical process type. The assert should check the physical process’ primary_type.

Suggested change

assert(self.primary_injection_process.primary_type == primary_interaction_collection.GetPrimaryType())

assert(self.primary_physical_process.primary_type == primary_interaction_collection.GetPrimaryType())

Copilot · 2026-04-28T04:37:04Z

+            for secondary_injection_process in self.secondary_injection_processes:
+                if secondary_injection_process.primary_type == secondary_type:
+                    inj_sec_defined = True
+            for secondary_physical_process in self.secondary_physical_processes:
+                if secondary_physical_process.primary_type == secondary_type:
+                    phys_sec_defined = True
+
            secondary_injection_process = _injection.SecondaryInjectionProcess()
            secondary_physical_process = _injection.PhysicalProcess()
            secondary_injection_process.primary_type = secondary_type
            secondary_physical_process.primary_type = secondary_type


When a secondary process for secondary_type already exists, this code still creates new secondary_injection_process/secondary_physical_process instances, adds distributions to those new instances, and then discards them (because they aren’t appended). That means existing processes won’t receive the intended distributions/updates. Instead, locate and reuse the existing process objects when *_sec_defined is true; only create new ones when missing.

Copilot · 2026-04-28T04:37:04Z

+            if not inj_sec_defined: self.secondary_injection_processes.append(secondary_injection_process)
+            if not phys_sec_defined: self.secondary_physical_processes.append(secondary_physical_process)


When a secondary process for secondary_type already exists, this code still creates new secondary_injection_process/secondary_physical_process instances, adds distributions to those new instances, and then discards them (because they aren’t appended). That means existing processes won’t receive the intended distributions/updates. Instead, locate and reuse the existing process objects when *_sec_defined is true; only create new ones when missing.

Copilot · 2026-04-28T04:37:04Z

+        if type(geo)==_geometry.Cylinder:
+            cylinder = _geometry.Cylinder(det_placement,geo.Radius,geo.InnerRadius,geo.Z)
+            return _distributions.CylinderVolumePositionDistribution(cylinder)
+        elif type(geo)==_geometry.Sphere:
+            sphere = _geometry.Sphere(det_placement,geo.Radius,geo.InnerRadius)
+            return _distributions.SphereVolumePositionDistribution(sphere)
+        else:
+            print("Geometry type %s not supported for position distribution. Exiting"%str(type(geo)))
+            exit(0)


Using type(x) == SomeType prevents subclasses/proxies from being recognized and is generally less robust than isinstance. Also, exit(0) again signals success on an error path; prefer raising NotImplementedError/ValueError so callers can handle unsupported geometry types programmatically.

Copilot · 2026-04-28T04:37:06Z

+        std::tuple<siren::math::Vector3D, siren::math::Vector3D> bounds;
+        if(datum->depth() == 0) {
+            std::get<0>(bounds) = datum->record.primary_initial_position; // start location
+            std::get<1>(bounds) = std::get<0>(injectors[i_inj]->PrimaryInjectionBounds(datum->record)); // start of injection bounds


i_inj is used to index injectors / *_weighters / maps without any bounds checking, which can cause undefined behavior (crash) if Python passes an invalid injector index. Additionally, catching std::out_of_range and returning {} after printing to stdout makes failures silent for Python callers. Validate i_inj up front and raise an exception (so pybind can translate it), or at minimum return an error in a way that the Python layer can detect reliably.

Copilot · 2026-04-28T04:37:06Z

+                survival_probs.push_back(1 - secondary_process_weighter_maps[i_inj].at(datum->record.signature.primary_type)->InteractionProbability(bounds, datum->record));
+            } catch(const std::out_of_range& oor) {
+                std::cout << "Out of Range error: " << oor.what() << '\n';
+                return {};
+            }


i_inj is used to index injectors / *_weighters / maps without any bounds checking, which can cause undefined behavior (crash) if Python passes an invalid injector index. Additionally, catching std::out_of_range and returning {} after printing to stdout makes failures silent for Python callers. Validate i_inj up front and raise an exception (so pybind can translate it), or at minimum return an error in a way that the Python layer can detect reliably.

Copilot · 2026-04-28T04:37:06Z

        for secondary_type, decay_list in secondary_decays.items():
-            # Define a sedcondary injection distribution
+
+            # Define a sedcondary injection distribution if necessary


Correct typo in comment: 'sedcondary' → 'secondary'.

Suggested change

# Define a sedcondary injection distribution if necessary

# Define a secondary injection distribution if necessary

Copilot · 2026-04-28T04:37:06Z

+                record = _dataclasses.InteractionRecord()
+                record.signature.primary_type = sec_inj.primary_type
+                found_collection = False
+                # Loop through possible seconday cross sections


Correct typo in comment: 'seconday' → 'secondary'.

Suggested change

# Loop through possible seconday cross sections

# Loop through possible secondary cross sections

Copilot · 2026-04-28T04:37:06Z

+    // Returns the vector of interaction physical probabilities for each process
+    // Since we are concerned only with the physical probability, we use the first injector since physical processes are the same for all injectors
+    // HOWEVER the injection bounds will change based on the injector
+    // so, we allow the user to specify which injector they are interseted in


Correct typo in comment: 'interseted' → 'interested'.

Suggested change

// so, we allow the user to specify which injector they are interseted in

// so, we allow the user to specify which injector they are interested in

austinschneider · 2026-04-28T15:32:25Z

Closing to reopen from the commit author's account so they can be added as a reviewer. Branch unchanged; will be re-linked from the new PR shortly.

Copilot AI review requested due to automatic review settings April 28, 2026 04:29

austinschneider force-pushed the feat/hnl-3body-sampling branch from 542e789 to ad12640 Compare April 28, 2026 04:34

austinschneider force-pushed the feat/controller-output branch from 008a996 to 7e97b68 Compare April 28, 2026 04:34

Copilot AI reviewed Apr 28, 2026

View reviewed changes

austinschneider force-pushed the feat/hnl-3body-sampling branch from ad12640 to c6019e8 Compare April 28, 2026 05:09

austinschneider force-pushed the feat/controller-output branch from 7e97b68 to 7166c83 Compare April 28, 2026 05:09

austinschneider force-pushed the feat/hnl-3body-sampling branch from c6019e8 to d6563e3 Compare April 28, 2026 06:04

austinschneider force-pushed the feat/controller-output branch from 7166c83 to 10542f0 Compare April 28, 2026 06:04

austinschneider force-pushed the feat/hnl-3body-sampling branch from d6563e3 to fd91564 Compare April 28, 2026 13:26

austinschneider force-pushed the feat/controller-output branch from 10542f0 to 75b41d9 Compare April 28, 2026 13:26

austinschneider force-pushed the feat/hnl-3body-sampling branch from fd91564 to 00db822 Compare April 28, 2026 14:36

austinschneider force-pushed the feat/controller-output branch from 75b41d9 to d724a51 Compare April 28, 2026 14:36

austinschneider closed this Apr 28, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Controller output features and integration fixes#112

Controller output features and integration fixes#112
austinschneider wants to merge 1 commit into
feat/hnl-3body-samplingfrom
feat/controller-output

austinschneider commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

austinschneider commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		print("Must provide either an experiment name or both a detector model file and materials model file. Exiting")
		exit(0)

	print("Must provide either an experiment name or both a detector model file and materials model file. Exiting")
	exit(0)
	raise ValueError("Must provide either an experiment name or both a detector model file and materials model file.")

	assert(self.primary_injection_process.primary_type == primary_interaction_collection.GetPrimaryType())
	assert(self.primary_physical_process.primary_type == primary_interaction_collection.GetPrimaryType())

		if not inj_sec_defined: self.secondary_injection_processes.append(secondary_injection_process)
		if not phys_sec_defined: self.secondary_physical_processes.append(secondary_physical_process)

	# Define a sedcondary injection distribution if necessary
	# Define a secondary injection distribution if necessary

	# Loop through possible seconday cross sections
	# Loop through possible secondary cross sections

	// so, we allow the user to specify which injector they are interseted in
	// so, we allow the user to specify which injector they are interested in

Uh oh!

Conversation

austinschneider commented Apr 28, 2026

Stack: PR 14 of 14

Squashed contents

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

austinschneider commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants