Add PrimaryExternalDistribution and bounded/physical vertex distributions by austinschneider · Pull Request #104 · Harvard-Neutrino/SIREN

austinschneider · 2026-04-28T04:28:58Z

Stack: PR 6 of 14

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

Base: feat/sphere-distribution
Head: feat/primary-external-distribution

Merge order: feat/sphere-distribution should land before this PR.

Squashed contents

Squashed diff for paths:

projects/distributions/private/primary/PrimaryExternalDistribution.cxx
projects/distributions/public/SIREN/distributions/primary/PrimaryExternalDistribution.h
projects/distributions/private/primary/vertex/PrimaryBoundedVertexDistribution.cxx
projects/distributions/private/primary/vertex/PrimaryPhysicalVertexDistribution.cxx
projects/distributions/public/SIREN/distributions/primary/vertex/PrimaryBoundedVertexDistribution.h
projects/distributions/public/SIREN/distributions/primary/vertex/PrimaryPhysicalVertexDistribution.h

Source commits on dev/HNL_DIS_clean that contributed:
21451da (Nicholas Kamp): first attempt at external list primary injector
34a229f (Nicholas Kamp): changes to fix runtime errors
455b073 (Nicholas Kamp): introduce primary bounded and physical vertex distributions to sample vertex in the case that the inital position is already provided from the external distribution
9098b42 (Nicholas Kamp): fix weighting issue in PrimaryExternalDist
a79d440 (Nicholas Kamp): getting HNLs to work for SINE and UNDINE
d7abebb (Nicholas Kamp): build fixes to PrimaryExternalDistribution
e530d33 (Nicholas Kamp): bug fix in PrimaryExternalDistribution

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: - projects/distributions/private/primary/PrimaryExternalDistribution.cxx - projects/distributions/public/SIREN/distributions/primary/PrimaryExternalDistribution.h - projects/distributions/private/primary/vertex/PrimaryBoundedVertexDistribution.cxx - projects/distributions/private/primary/vertex/PrimaryPhysicalVertexDistribution.cxx - projects/distributions/public/SIREN/distributions/primary/vertex/PrimaryBoundedVertexDistribution.h - projects/distributions/public/SIREN/distributions/primary/vertex/PrimaryPhysicalVertexDistribution.h Source commits on dev/HNL_DIS_clean that contributed: 21451da (Nicholas Kamp): first attempt at external list primary injector 34a229f (Nicholas Kamp): changes to fix runtime errors 455b073 (Nicholas Kamp): introduce primary bounded and physical vertex distributions to sample vertex in the case that the inital position is already provided from the external distribution 9098b42 (Nicholas Kamp): fix weighting issue in PrimaryExternalDist a79d440 (Nicholas Kamp): getting HNLs to work for SINE and UNDINE d7abebb (Nicholas Kamp): build fixes to PrimaryExternalDistribution e530d33 (Nicholas Kamp): bug fix in PrimaryExternalDistribution

Copilot

Pull request overview

Note

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

Adds new primary injection distributions for externally-specified primaries and for sampling interaction vertices given an initial position, enabling bounded and physically-weighted vertex generation.

Changes:

Introduces PrimaryExternalDistribution to sample primary properties from an external CSV-like input.
Adds PrimaryBoundedVertexDistribution to sample vertices along a finite path, optionally restricted to a fiducial volume.
Adds PrimaryPhysicalVertexDistribution to sample vertices according to interaction depth along the detector path.

Reviewed changes

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

Show a summary per file

File	Description
projects/distributions/public/SIREN/distributions/primary/PrimaryExternalDistribution.h	Declares external-file-driven primary injection distribution and cereal registration.
projects/distributions/private/primary/PrimaryExternalDistribution.cxx	Implements file parsing, sampling logic, cloning, and comparison methods.
projects/distributions/public/SIREN/distributions/primary/vertex/PrimaryBoundedVertexDistribution.h	Declares bounded vertex sampler with optional fiducial-volume restriction and cereal IO.
projects/distributions/private/primary/vertex/PrimaryBoundedVertexDistribution.cxx	Implements bounded vertex sampling & probability density calculations along a path.
projects/distributions/public/SIREN/distributions/primary/vertex/PrimaryPhysicalVertexDistribution.h	Declares physically-weighted vertex sampler and cereal registration.
projects/distributions/private/primary/vertex/PrimaryPhysicalVertexDistribution.cxx	Implements physically-weighted vertex sampling & probability density calculations along a path.

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

Copilot · 2026-04-28T04:36:57Z

+#include <string>                                        // for string
+#include <vector>                                        // for vector
+#include <cstdint>                                       // for uint32_t
+#include <stdexcept>                                     // for runtime_error


std::ifstream is used but <fstream> is not included in this header (and it’s not included in the .cxx either). This will fail to compile in translation units that include this header without already including <fstream>. Add #include <fstream>.

Suggested change

#include <stdexcept> // for runtime_error

#include <stdexcept> // for runtime_error

#include <fstream> // for ifstream

Copilot · 2026-04-28T04:36:58Z

+    };
+    std::string filename;
+private:
+    std::ifstream input_file;


std::ifstream is used but <fstream> is not included in this header (and it’s not included in the .cxx either). This will fail to compile in translation units that include this header without already including <fstream>. Add #include <fstream>.

Copilot · 2026-04-28T04:36:58Z

@@ -0,0 +1,171 @@
+#include "SIREN/distributions/primary/PrimaryExternalDistribution.h"
+
+#include <array>                                           // for array


This file uses std::stringstream, std::cerr, and exit(...) but does not include <sstream>, <iostream>, or <cstdlib>. Relying on transitive includes is brittle and can break builds. Add the missing standard headers explicitly.

Suggested change

#include <array> // for array

#include <array> // for array

#include <cstdlib> // for exit

#include <iostream> // for cerr

#include <sstream> // for stringstream

Copilot · 2026-04-28T04:36:58Z

@@ -0,0 +1,171 @@
+#include "SIREN/distributions/primary/PrimaryExternalDistribution.h"
+
+#include <array>                                           // for array


This file uses std::stringstream, std::cerr, and exit(...) but does not include <sstream>, <iostream>, or <cstdlib>. Relying on transitive includes is brittle and can break builds. Add the missing standard headers explicitly.

Suggested change

#include <array> // for array

#include <array> // for array

#include <cstdlib> // for exit

#include <iostream> // for cerr

#include <sstream> // for stringstream

Copilot · 2026-04-28T04:36:58Z

+{
+    PrimaryExternalDistribution(other.filename);


The copy constructor is effectively a no-op: it constructs and immediately discards a temporary PrimaryExternalDistribution, leaving this uninitialized (and likely with default emin, empty input_data, etc.). Implement this as a delegating constructor (e.g., initialize filename/emin and call LoadInputFile) or explicitly copy the relevant members.

Suggested change

{

PrimaryExternalDistribution(other.filename);

: emin(other.emin)

{

filename = other.filename;

init_pos_set = other.init_pos_set;

mom_set = other.mom_set;

keys = other.keys;

input_data = other.input_data;

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

+    double prob_density;
+    if(total_interaction_depth < 1e-6) {
+        prob_density = interaction_density / total_interaction_depth;
+    } else {


total_interaction_depth can be 0 (e.g., no targets / zero cross sections in bounds), which makes the < 1e-6 branch divide by zero. SamplePosition() explicitly throws on total_interaction_depth == 0, but GenerationProbability() should also handle this case (typically returning 0.0) before dividing.

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

+bool PrimaryBoundedVertexDistribution::less(WeightableDistribution const & other) const {
+    const PrimaryBoundedVertexDistribution* x = dynamic_cast<const PrimaryBoundedVertexDistribution*>(&other);
+    return
+        std::tie(max_length)
+        <
+        std::tie(x->max_length);
+}


Potential null dereference: if other is a different distribution type, x is null and x->max_length is UB. Also, equal()/less() only consider max_length and ignore fiducial_volume, even though it changes sampling/probabilities; that can break caching/lookup behavior for weightable distributions. Handle !x deterministically (type ordering) and incorporate a meaningful fiducial-volume identity (or document why it’s intentionally excluded).

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

+
+    double interaction_density = detector_model->GetInteractionDensity(path.GetIntersections(), DetectorPosition(vertex), targets, total_cross_sections, total_decay_length);
+
+    double prob_density;


Same division-by-zero issue as in PrimaryBoundedVertexDistribution: if total_interaction_depth == 0, the < 1e-6 branch divides by zero. Add an explicit if (total_interaction_depth == 0) return 0.0; (or equivalent) before this block.

Suggested change

double prob_density;

double prob_density;

if(total_interaction_depth == 0.0)

return 0.0;

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

+}
+
+bool PrimaryPhysicalVertexDistribution::less(WeightableDistribution const & other) const {
+    const PrimaryPhysicalVertexDistribution* x = dynamic_cast<const PrimaryPhysicalVertexDistribution*>(&other);


Returning false unconditionally does not define a strict weak ordering and can break ordered containers / comparison-based registries if WeightableDistribution::less is used that way. Implement a deterministic ordering: handle !x via type-based ordering, and for same-type objects either return false only if they are truly indistinguishable (and ensure equal() matches), or compare relevant configuration fields.

Suggested change

const PrimaryPhysicalVertexDistribution* x = dynamic_cast<const PrimaryPhysicalVertexDistribution*>(&other);

const PrimaryPhysicalVertexDistribution* x = dynamic_cast<const PrimaryPhysicalVertexDistribution*>(&other);

if(!x)

return Name() < other.Name();

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

+}
+
+} // namespace distributions
+} // namespace sirenREN


Typo in namespace closing comment: sirenREN should be siren.

Suggested change

} // namespace sirenREN

} // namespace siren

austinschneider · 2026-04-28T15:31:55Z

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:28

austinschneider force-pushed the feat/sphere-distribution branch from 582b384 to 52bfd20 Compare April 28, 2026 04:34

austinschneider force-pushed the feat/primary-external-distribution branch from cc05a02 to 4a078b6 Compare April 28, 2026 04:34

Copilot AI reviewed Apr 28, 2026

View reviewed changes

austinschneider force-pushed the feat/sphere-distribution branch from 52bfd20 to 4a3b65e Compare April 28, 2026 05:09

austinschneider force-pushed the feat/primary-external-distribution branch 2 times, most recently from 4500aec to e4231d5 Compare April 28, 2026 06:04

Copilot started reviewing on behalf of austinschneider April 28, 2026 11:24 View session

austinschneider mentioned this pull request Apr 28, 2026

HNL 2-body decays #109

Closed

austinschneider force-pushed the feat/sphere-distribution branch from 4a3b65e to 924e07b Compare April 28, 2026 13:26

austinschneider force-pushed the feat/primary-external-distribution branch 2 times, most recently from 05bc3dd to c967cc7 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!

Add PrimaryExternalDistribution and bounded/physical vertex distributions#104

Add PrimaryExternalDistribution and bounded/physical vertex distributions#104
austinschneider wants to merge 1 commit into
feat/sphere-distributionfrom
feat/primary-external-distribution

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

	#include <stdexcept> // for runtime_error
	#include <stdexcept> // for runtime_error
	#include <fstream> // for ifstream

		@@ -0,0 +1,171 @@
		#include "SIREN/distributions/primary/PrimaryExternalDistribution.h"

		#include <array> // for array

-#include <array>                                           // for array
+#include <array>                                           // for array
+#include <cstdlib>                                         // for exit
+#include <iostream>                                        // for cerr
+#include <sstream>                                         // for stringstream

-{
-    PrimaryExternalDistribution(other.filename);
+    : emin(other.emin)
+{
+    filename = other.filename;
+    init_pos_set = other.init_pos_set;
+    mom_set = other.mom_set;
+    keys = other.keys;
+    input_data = other.input_data;


		double interaction_density = detector_model->GetInteractionDensity(path.GetIntersections(), DetectorPosition(vertex), targets, total_cross_sections, total_decay_length);

		double prob_density;

Uh oh!

Conversation

austinschneider commented Apr 28, 2026

Stack: PR 6 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