DetectorModel: query sectors by name, fiducial volume support by austinschneider · Pull Request #106 · Harvard-Neutrino/SIREN

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

Stack: PR 8 of 14

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

Base: feat/2d-flux-tables
Head: feat/detector-sector-by-name

Merge order: feat/2d-flux-tables should land before this PR.

Squashed contents

Squashed diff for paths:

projects/detector/private/DetectorModel.cxx
projects/detector/private/Path.cxx
projects/detector/private/pybindings/DetectorModel.h
projects/detector/public/SIREN/detector/DetectorModel.h

Source commits on dev/HNL_DIS_clean that contributed:
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
c4b1ec0 (Nicholas Kamp): add fiducial volume to LG and enable querying of detector sectors by name
f6f8b46 (Nicholas Kamp): get the decay sampling working

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/detector/private/DetectorModel.cxx - projects/detector/private/Path.cxx - projects/detector/private/pybindings/DetectorModel.h - projects/detector/public/SIREN/detector/DetectorModel.h Source commits on dev/HNL_DIS_clean that contributed: 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 c4b1ec0 (Nicholas Kamp): add fiducial volume to LG and enable querying of detector sectors by name f6f8b46 (Nicholas Kamp): get the decay sampling working

Copilot

Pull request overview

Note

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

Adds sector lookup by name to DetectorModel, improves numerical handling for very short decay lengths, and short-circuits sector loops for decay-only cases.

Changes:

Add DetectorModel::GetSector(std::string name) const and expose the overload in pybind11.
Add a rounding-error guard in Path::IsWithinBounds for very small decay lengths.
Early-return optimizations for decay-only computations in interaction density/depth calculations.

Reviewed changes

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

File	Description
projects/detector/public/SIREN/detector/DetectorModel.h	Adds a new public `GetSector(name)` overload.
projects/detector/private/pybindings/DetectorModel.h	Binds the `GetSector(int)` and `GetSector(string)` overloads explicitly in Python.
projects/detector/private/Path.cxx	Adds a small-distance clamp to reduce rounding issues in bounds checks.
projects/detector/private/DetectorModel.cxx	Implements name-based sector lookup and adds decay-only early returns in interaction computations.

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

Copilot · 2026-04-28T04:35:51Z

+double estimateDotError(const std::array<double, 3>& p0, const std::array<double, 3>& p1, const std::array<double, 3>& int_dir) {
+    constexpr double epsilon = std::numeric_limits<double>::epsilon();
+
+    double dot_product = 0.0;
+    double error_sum = 0.0;
+
+    for (int i = 0; i < 3; ++i) {
+        double diff = p1[i] - p0[i];
+        double prod = int_dir[i] * diff;
+        dot_product += prod;
+        error_sum += std::fabs(int_dir[i] * diff);
+    }
+
+    // Estimated error of the expression
+    double error_estimate = epsilon * error_sum;
+
+    return error_estimate;
+}


estimateDotError is introduced but not used (the only call site is commented out later), which can trigger unused-function warnings (often fatal under -Werror) and adds dead code. Either remove it, or mark it as intentionally unused (e.g., [[maybe_unused]]) until it’s actually part of the active precision check.

Copilot · 2026-04-28T04:35:51Z

+DetectorSector DetectorModel::GetSector(std::string name) const {
+    for (auto sector : sectors_) {
+        if (sector.name==name) return sector;
+    }
+    std::cout << "Sector " << name << " not found, returning empty sector\n";
+    return DetectorSector();
+}


This API silently returns a default DetectorSector() on lookup failure and prints to std::cout, which is a surprising side-effect for a library call and makes failures easy to miss (and hard to handle from C++/Python callers). Prefer returning an error signal that callers can act on (e.g., throw std::out_of_range/std::runtime_error, or return std::optional<DetectorSector> / a pointer), and avoid writing to stdout from the library. Also avoid copying each sector: iterate by const auto& and accept const std::string& (or std::string_view) to prevent unnecessary copies.

Copilot · 2026-04-28T04:35:51Z

+    // If we have only decays, avoid the sector loop
+    if(targets.empty()) {
+      return distance/total_decay_length; // m / m --> dimensionless
+    }
    if(distance == 0.0) {
        return 0.0;
    }


The decay-only early return occurs before the distance == 0.0 guard. For decay-only paths where p0 == p1, this changes behavior from returning 0.0 to returning 0.0 / total_decay_length, which can become NaN if total_decay_length is also 0. Move the distance == 0.0 check before the decay-only return (or explicitly handle distance == 0.0 inside the decay-only branch).

Copilot · 2026-04-28T04:35:52Z

+        // avoid rounding errors for very small decay lengths; assume mm precision is fine
+        if((first_point_det_.get() - point).magnitude() < 1e-3) d0 = 0;
        return d0 <= 0 and d1 >= 0;


The rounding-error clamp is applied only to d0 (relative to first_point_det_), but the same failure mode can occur near last_point_det_ causing d1 to be slightly negative and incorrectly fail d1 >= 0. Consider applying a symmetric clamp for d1 as well. Also, 1e-3 is a magic number—use a named tolerance (and ensure the unit is consistent with the coordinate system) to make the intended precision explicit.

Copilot · 2026-04-28T04:35:52Z


    void AddSector(DetectorSector sector);
    DetectorSector GetSector(int level) const;
+    DetectorSector GetSector(std::string name) const;


Passing name by value forces an unnecessary string copy on each call. Prefer const std::string& (or std::string_view if appropriate for the API) to avoid allocations/copies, especially since the implementation also only reads the parameter.

Suggested change

DetectorSector GetSector(std::string name) const;

DetectorSector GetSector(std::string const & name) const;

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

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/2d-flux-tables branch from b9c0966 to 496977b Compare April 28, 2026 04:34

austinschneider force-pushed the feat/detector-sector-by-name branch from b946500 to a1ecea2 Compare April 28, 2026 04:34

Copilot AI reviewed Apr 28, 2026

View reviewed changes

austinschneider force-pushed the feat/2d-flux-tables branch from 496977b to 967cc0a Compare April 28, 2026 05:09

austinschneider force-pushed the feat/detector-sector-by-name branch from a1ecea2 to 27c1fa6 Compare April 28, 2026 05:09

austinschneider force-pushed the feat/2d-flux-tables branch from 967cc0a to 9204f13 Compare April 28, 2026 06:04

austinschneider force-pushed the feat/detector-sector-by-name branch from 27c1fa6 to 6da3cb1 Compare April 28, 2026 06:04

austinschneider force-pushed the feat/2d-flux-tables branch from 9204f13 to b717fce Compare April 28, 2026 13:26

austinschneider force-pushed the feat/detector-sector-by-name branch from 6da3cb1 to 39252ee Compare April 28, 2026 13:26

Copilot started reviewing on behalf of austinschneider April 28, 2026 14:15 View session

austinschneider force-pushed the feat/2d-flux-tables branch from b717fce to c04ebb4 Compare April 28, 2026 14:36

austinschneider force-pushed the feat/detector-sector-by-name branch from 39252ee to daab60e 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!

DetectorModel: query sectors by name, fiducial volume support#106

DetectorModel: query sectors by name, fiducial volume support#106
austinschneider wants to merge 1 commit into
feat/2d-flux-tablesfrom
feat/detector-sector-by-name

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!

austinschneider commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	DetectorSector GetSector(std::string name) const;
	DetectorSector GetSector(std::string const & name) const;

Uh oh!

Conversation

austinschneider commented Apr 28, 2026

Stack: PR 8 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!

austinschneider commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants