Draft
Conversation
- Replace Blob*-keyed map with vector indexed by blob position (determinism) - Hoist scope-transform computation out of per-step path-check inner loops - Replace double-lookup closest_index update with operator[]+std::next erase - Move early return before std::sort (skip wasted sort when num<=1) - Add explicit parentheses to all ||/&& ambiguous conditions; remove pragma - Delete dead scope_name variable and unused num_edges counter - Add review doc: clus/docs/clustering/clustering_protect_overclustering_review.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add clus/docs/patternrecognition/do_multi_tracking_review.md: full port review comparing multi-segment tracking against the uBooNE prototype, covering correctness, efficiency, determinism, and multi-APA/face geometry. - Fix §2.2: frame-mixing bug in multi_trajectory_fit Gaussian charge weighting — backward() transform now uses the pixel's (apa, face) instead of the 3D point's own face. - Fix §2.4: vertex projection after fit_point now re-queries contained_by() on the fitted position so cross-APA vertex moves use the correct geometry. - Fix §4.1: update_association signature extended to accept pre-built all_segments vector from caller, eliminating O(N×M) per-call rebuilds. - Fix §4.2: FaceGeom cache built once per multi_trajectory_fit call, replacing per-vertex/segment double wpid_offsets+wpid_slopes map lookups. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix steiner_flags_uint8 type: std::vector<int> → std::vector<uint8_t> (4× memory saving for large point clouds) - Remove unpopulated m_steiner_graph_terminal_indices member from SteinerGrapher.h; leave note for future recover_steiner_graph() port - Remove dead commented-out debug/cout blocks from CreateSteinerGraph.cxx Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add examine_graph_review.md auditing Examine_graph /
Connect_graph_overclustering_protection against the WCP prototype.
Bug fixes (connect_graph_relaxed.cxx):
- B.1: rekey wpid_U/V/W_dir maps by {apa,face} pair instead of full
WirePlaneId — the old key caused std::out_of_range on multi-APA events
when contained_by() returned a different layer than wpids() emitted.
- B.2: count path steps between APA volumes (apa==-1) as bad instead of
silently skipping them; applied to main loop, dir1 loop, and dir2 loop.
- B.3: same fix in check_connectivity.
- B.6: collapse three dead if/else blocks (both branches identical) into
a single const assignment.
Bug fixes (clustering_protect_overclustering.cxx):
- B.4: add cross-reference comment to Separate_overclustering() directing
maintainers to keep it in sync with connect_graph_relaxed.
- B.5: reserve(num) not reserve(component.size()) for ordered_components.
- B.2-equiv: count out-of-volume steps as bad in check_path lambda.
- B.6: collapse three dead if/else blocks.
Efficiency improvements:
- C.1 (connect_graph_closely.cxx): replace BlobLess-keyed std::map with
std::unordered_map<Blob*, ..., BlobPtrHash> for O(1) blob lookups.
Applied to both connect_graph_closely and connect_graph_closely_pid.
- C.2 (clustering_protect_overclustering.cxx): wire-interval range scan
now uses lower_bound/upper_bound instead of a linear walk.
- C.3 (both files): eliminate redundant num^2 sentinel-initialization
loop by constructing the 5 distance arrays with sentinel values directly.
- C.4 (connect_graph_relaxed.cxx): skip vhough_transform calls when the
closest-pair distance >= 80 cm (the probe range of
get_closest_point_along_vec), avoiding O(num^2) Hough work for distant
component pairs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add clus/docs/patternrecognition/pid_direction_kinematics_review.md reviewing ten prototype WCPPID functions (do_track_comp, eval_ks_ratio, do_track_pid, cal_kine_dQdx, cal_kine_range, cal_4mom, determine_dir_track, determine_dir_shower_trajectory, determine_shower_direction, break_segment_at_point) and the WCShower helpers against the toolkit equivalents in PRSegmentFunctions.cxx and PRShower.cxx. Code fixes applied (PRSegmentFunctions.cxx): - P1: clamp all 8 std::acos dot-product arguments to [-1,1] in segment_determine_shower_direction to prevent NaN propagation - P2/P3: fix segment_cal_kine_dQdx endpoint dX shortening — compute dE/dx from original fits[i].dx (Box model is non-linear); apply saturation filter against fits[i].dx not the shortened value - P4: replace terse WARNING in break_segment with expanded comment explaining find_vertices already disambiguates orientation via wcpts.front() distance comparison - P6: restore if(flag_print) guard on SPDLOG_LOGGER_DEBUG call in segment_determine_dir_track (was accidentally commented out) - P7/P11: remove segment->dirsign(0) side-effect from failure path of segment_do_track_pid; add ncount==0 early-return in segment_do_track_comp - P13: add multi-APA limitation comment above segment_cal_kine_dQdx - P14: add unit-convention comment above dQ_dx[i] in segment_determine_dir_track - P15: cache segment_median_dQ_dx once across three overlapping calls in segment_determine_dir_track - B8.1/Q6: replace "hack for now" comment in segment_determine_shower_direction_trajectory with explanation that pdg=11 and particle_score=100.0 are intentional sentinels Code fixes applied (PRShower.cxx): - P16: replace (0,0,0) origin sentinel in calculate_kinematics with dist>=0 check on shower_get_closest_point return value - B17.1: add invariant comment above early-return guard in calculate_kinematics_long_muon explaining why it is logically unreachable (call-site invariant: start segment always has particle_info() with pdg=±13 before this function is reached) Documentation updates: - clus/docs/porting/porting_dictionary.md: add cross-reference to new review doc under Trajectory Fitting section - clus/docs/porting/neutrino_id_function_map.md: add row entries for all ten functions and pointer to review doc Verified (no code change needed): P5 (D4Vector callers use named accessors), P8 (qlport/uboone-mabc.jsonnet A=1.0), P10 (kenergy_best migration complete), P12 (resolved by P2/P3), P17 (calculate_ kinematics_long_muon audited), P18 (DynamicPointCloud::add_points correctly face-aware). P9 (reference table provenance) deferred. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…int) Storing the flag as uint8_t caused a WireCell::ValueError "element size mismatch 4 != 1" whenever any reader called ->elements<int>(). Store as int instead so the array dtype matches all call sites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds §2.2 to steiner_graph_review.md describing the element-size mismatch bug introduced by the §8 cosmetic note and the fix applied in the previous commit. Corrects the misleading §8 entry that described the change backwards. 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
2 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