Draft
Conversation
Document 6 critical, 14 moderate, 12 minor findings across the aux package signal processing code. No source code modified. Top critical issues: DftTools::replace() uses wrong domain vectors, BlobShadow::connected_leaves() pushes wrong vertex type, ClusterArrays activity vector sized by wrong dimension. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bug fixes (17 of 40 identified): - Critical: global PMT_ROIs -> member variable (thread safety) - Critical: min variable never updated in PMT peak-finding loop - Critical: L1SP reverse-scan loop index mismatch (wrong ROIs processed) - Critical: swapped min/max in Protodune LinearInterpSticky - Critical: PeakFinding uninitialized pointers, memory leak on reuse - Medium: 6 division-by-zero guards (apply_roi, BreakROI, RemovePMTSignal, RawAdapativeBaselineAlg in 4 detector files) - Medium: dead merge loop in find_ROI_loose (flag_repeat=0 -> 1) - Medium: unsigned underflow in section_protected loop - Medium: L1SP flag classification && operand order (short-circuit fix) - Medium: NaN clamp in Chirp RMS sqrt, Partial spectrum OOB guard - Low: config key typos (r_th_precent, isWarped), float->int wrap comparison Efficiency improvements (11 of 25 identified): - cal_RMS: pass-by-value -> const& (eliminates ~1GB copies/frame) - L1SP: cache 20 JSON config values + smearing vector in configure() - OmnibusNoiseFilter: const auto& in range-for loops, unordered_set for bad channel lookup - OmnibusPMTNoiseFilter: eliminate double waveform copy per trace - ROI_formation/refinement: cache bad_ch_map lookups in inner loops - CalcRMSWithFlags: vector reserve() in 4 detector files - get_mp2_rois(): return by reference instead of by value Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bug fixes (21): - RootfileCreation: wrong factory IDepoFilter→IFrameFilter - MagnifySource: null TFile/histogram checks, TFile leak - CelltreeFrameSink: channel ID as vector index, mask vector misalignment, memory leaks - CelltreeSource: memory leaks in read_traces/read_cmm, TFile leaks - UbooneTTrees: flag_uvw null pointer capture, flash vector oversizing, parent_cluster_id fallback - UbooneClusterSource: missing negation in optical condition, empty slice/blob check - UbooneBlobSource: double EOS insertion - BDT scorers: log-odds division by zero, numu_3 missing fill gate, include guard typo - MagnifySink: summary histogram bin range - Tracking app: dtheta out-of-bounds for single-point clusters - Jsonnet: "weiner"→"wiener" typo Efficiency improvements (7): - BDT scorers: move TMVA::Reader/BookMVA to configure() (10-100x speedup) - CelltreeSource: open file once, pass TTree* to read functions - HistFrameSink: SetBinContent instead of Fill (avoid Sumw2 bloat) - UbooneClusterSource: direct slice range for bad channels, O(N) cluster dispatch - MagnifySink: const ref for shared_ptr iteration - TrackingVisitor: O(E+V) per-cluster edge/vertex maps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds walk-history to proto_extend_point and uses the halfway walk point as break_pt when dot(dir1, dir1_prev) < -0.5, preventing spurious zig-zag stubs from near-front kinks. Raises dl_vtx_cut default to 2.5 cm (was 2.0 cm in TaggerCheckNeutrino default_configuration) and removes the redundant dl_vtx_cut parameter from clus.jsonnet so C++ is the single source of truth. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e_shower_1
In examine_shower_1, when building a candidate shower starting from a weakly-
directed muon track (sg) at main_vertex, complete_structure_with_start_segment
flood-fills the graph and pulls in any same-cluster neighbor — including a
Michel electron shower (Shower_C) attached at sg's far-end junction. The
inner associated-showers loop then finds Shower_C via the angle/distance test
(angle ≈ 0° and distance = 0 because they share a vertex), satisfies the
energy/length commit condition, and flips sg's pdg from 13 → 11, converting
the stopping muon into an electron.
Fix: in the low-energy else-branch of the associated-showers loop, skip any
conn_type=1 shower whose start_vertex is already inside shower1's vertex set.
Such a shower is a downstream decay product (e.g. a Michel electron starting
at the muon's stopping vertex), not an external EM shower that should cause the
start segment to be reclassified. High-energy (>80 MeV) downstream showers
are handled by the existing if-branch above and are unaffected.
Also carries forward two earlier fixes committed to examine_shower_1:
- set_mass(electron) when flipping start_segment pdg to 11 (mass consistency)
- dedup loop to remove stale single-segment muon showers that share a
start_segment with the newly created shower1 (prevents duplicate bee ids)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In fill_bee_pf_tree, indirect-connection showers (conn_type 2/3) get a pseudo mother node to bridge the gap to the main vertex. The prototype (fill_psuedo_reco_tree, fill_ssmsp_psuedo overloads 1 & 3) labels this pseudo mother as gamma (22) for EM showers and neutron (2112) for all others — the physical reading being that an isolated proton activity visible only weakly must have been produced by an unseen neutral (neutron). The toolkit's SSM tagger (fill_ssmsp_pseudo_1/_3) already applied this rule correctly. The Bee PF tree helper (append_gamma_shower) did not: it always wrote "gamma" regardless of the underlying shower's particle type, silently mislabeling isolated proton activities. The pf_pdg_to_name helper already had case 2112 → "n" but it was never reached. Fix: rename append_gamma_shower → append_pseudo_shower and add the PDG test (e/γ → 22, otherwise → 2112) matching the SSM and prototype logic. Pi0 children are unaffected (they are always EM, condition evaluates to 22). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n break_segments segment_search_kink's flag_switch direction-swap trigger had no minimum-window guard: when a kink sat ~2-3 cm from a track endpoint, only 3-4 post-kink fit points existed and were trivially collinear (path≈chord to within 0.03), causing flag_switch to fire on degeneracy rather than real geometry. This dispatched proto_extend_point backward into the track body, producing spurious ~2-3 cm tail segments at track endpoints. Fix 1 (PRSegmentFunctions.cxx, segment_search_kink): require the full 9-point post-kink window AND an absolute chord > 3 cm before trusting the straightness ratio. Both branches (flag_switch and flag_search else-if) are guarded symmetrically. Fix 2 (NeutrinoPatternBase.cxx, break_segments geometry check): for degree-1 end vertices, extend the replace_segment_and_vertex condition from (min_dis < 1.5 cm && angle > 120°) to (min_dis < 2.0 cm && kink_angle_at_break > 30°). kink_angle_at_break measures the approach-vs- departure angle in the steiner path at break_wcp, which is a more physically meaningful discriminant than the overall-segment angle used previously. This absorbs the residual ~1.7 cm tail that survives after Fix 1 when flag_continue is false and the walker snaps to the kink itself. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nalysis Replace the previous 13-component (5 live, 8 commented-out) composite score with a clean 7-component formula tuned on 36 annotated events: score = s_dl + s_snap + s_fwd_z + s_clen + s_isol + s_main + s_fv Key changes vs. the previous scheme: - Re-enable s_clen (cluster length bonus, +2.0 max at 60 cm) and boost s_main (+2.0) and s_isol (-2.0) so geometric signals can compete in the uncertain-DL regime (where s_dl ≈ 5 for all candidates). - Cap s_fwd_z at ±0.25 (was -(z-z_min)/200cm, reaching -3 on wide events) so it can no longer swamp the main-cluster bonus. - Replace the hard 2*dl_vtx_cut snap-distance gate with a soft s_snap penalty (-min(2, snap/5cm)), fixing 3 false rejections on long main-cluster candidates at 5-8 cm snap distance. - Delete all dead commented-out scoring code (segs, ltrk, mult, flg_in, conf, ptbk) per "delete unused code" policy. - Add DL score mean/std/regime log line for future diagnosis. - Bump dl_vtx_min_accept_score default 0.0 → 4.0: empirically separates correct uncertain-regime picks (8-12) from true-failure events (3-5). Net effect on the 36-event log: +6 correct outcomes (3 wrong picks flipped, 3 false rejections accepted), 0 regressions. Also includes Phase 1 changes (top-K DL payload from Python, dl_vtx_rerank toggle, dl_vtx_score_scale knob). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The degree-1 terminus branch of use_replace in break_segments (introduced in 7725a9c to absorb ~1.7 cm fold-back tail artifacts) was also absorbing genuine Michel electrons. Diagnostics on two events revealed the key discriminant: Event 6852 (fold-back tail): tv1-vs-tv2 angle = 27.8°, use_replace=1 ✓ Event 6528 (12.94 MeV Michel): tv1-vs-tv2 angle = 79.2°, use_replace=1 ✗ tv1 = end_v - start_v (whole segment direction) tv2 = end_v - break_wcp (local stub direction) A fold-back artifact at a track endpoint has the stub pointing in roughly the same direction as the main track (small angle), because the fitter has simply extended a few points past the true end. A real physical secondary (Michel electron, delta ray) diverges from the parent axis at a significant angle. kink_angle_at_break cannot discriminate (both cases ≈50–90°). Add `angle < 45°` to the terminus condition: - fold-back tail (27.8°): still absorbed ✓ - Michel electron (79.2°): falls through to break_segment_into_two ✓ Also removes the temporary std::cout diagnostic prints and #include<iostream> added to PRSegmentFunctions.cxx during investigation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…a output Previously m_fitted_charge_2d was cleared at the start of every fill_fitted_charge_2d() call, so T_proj_data only captured the last cluster-filtered do_multi_tracking() pass — all other beam-flash clusters were silently dropped. Add a per-cluster snapshot cache m_cluster_fitted_charge_2d that stores the result of each fill_fitted_charge_2d() call keyed by m_cluster_filter. Overwriting the same key on each re-fit gives "latest fit wins per cluster", handling the case where a cluster is re-fit during examine_structure / merge_vertices. A new assemble_fitted_charge_2d() method merges all snapshots into m_fitted_charge_2d at the end; TaggerCheckNeutrino::visit calls it just before grouping.set_track_fitting() so write_proj_data sees all clusters' cells with correct cluster_id and pred_charge. Also consolidate kPlaneChOffset usage in UbooneMagnifyTrackingVisitor and apply it consistently to pu/pv/pw fields in T_rec_charge. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New IEnsembleVisitor that opens the existing tracking ROOT file in UPDATE mode and adds T_tagger (all TaggerInfo fields + nu_x/y/z) and T_kine (all KineInfo fields), matching prototype output tree names. Also adds tagger_output() factory to clus.jsonnet and a tagger validation plan document. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New comparison binary that reads T_tagger and T_kine from a prototype ROOT file and a toolkit ROOT file, compares all scalar (Float_t/Int_t) and vector (vector<float>/vector<int>) branches event by event, and groups results by originating tagger function (cosmic, gap, mip, ssm, shw_sp, pio_family, stem, lem, br, tro, hol_lol, vis, numu, nue, kine, etc.). Writes per-category histograms and a T_summary tree to report.root; prints a compact per-category table and exits non-zero if any branch differs. Replaces the earlier separate build-verify and single-event manual inspection steps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Section 5.4 now documents the actual C++ comparison app (wire-cell-uboone-tagger-compare) replacing the earlier Python script placeholder. Implementation sequence table updated with DONE/TODO status for each step. Key file reference table extended with the new visitor header/impl and comparison app. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make this new one, as #444 seems not working for the tracking purpose