engine: implement strongest-path loop discovery algorithm#249
Conversation
Implement the "Finding the Loops That Matter" strongest-path heuristic from Eberlein & Schoenberg (2020) Appendix I. This provides an alternative to exhaustive loop enumeration that scales to large models by using link score magnitudes to guide a DFS search. New module ltm_finding.rs contains the core algorithm: for each timestep, starting from each stock, follow the strongest outbound link scores to discover the most influential feedback loops. Uses strict less-than pruning with best_score persistence across stock iterations within a timestep, and deduplicates loops by sorted node set. Integration changes: CausalGraph.all_links() returns every causal edge, generate_ltm_variables_all_links() instruments all links (not just loop links), and Project.with_ltm_all_links() wires it together. This "discovery mode" generates link scores for all edges so the strongest-path search can operate over the full causal structure. Test models: three-party arms race (7 loops: 3 self-balancing, 3 pairwise reinforcing, 1 three-way reinforcing with reverse deduplicated) and decoupled stocks (time-varying IF-THEN-ELSE loop activation). Cross-validation confirms discovery finds all loops in small models.
bpowers
force-pushed
the
bobby/finding-loops-that-matter
branch
from
d02efdb to
56a5847
Compare
Code Review: Strongest-Path Loop Discovery AlgorithmSummaryThis PR implements the "loops that matter" strongest-path discovery algorithm from Eberlein & Schoenberg (2020). The implementation is well-documented, thoroughly tested, and follows the codebase conventions. Overall, this is high-quality work with excellent test coverage and clear design choices. Strengths1. Excellent Documentation
2. Strong Test Coverage
3. Good Architecture
4. Performance-Conscious Design
Issues and Concerns1. CRITICAL: Thread Safety in Multi-Threaded ContextsLocation: Issue: The algorithm relies on Recommendation:
Severity: Medium (depends on future parallelization plans) 2. Code Quality: Redundant .abs() CallLocation: Issue: Recommendation: Remove the Severity: Low (correctness is fine, but code clarity suffers) 3. Potential Performance Issue: Quadratic String AllocationsLocation: Issue: This allocates strings for node sets twice: once at line 324 and again at line 237 within Recommendation: Use a helper function to avoid duplication. Severity: Low (likely negligible in practice, but cleaner code) 4. Test Brittleness: Hard-Coded Loop CountLocation: Issue: The test expects exactly 7 loops with a detailed comment about deduplication. If the deduplication logic changes or the model is modified, this test will break without clear indication of whether the behavior is correct. Recommendation: Add a comment documenting the expected loop structure OR make the test more flexible. Severity: Low (test maintenance concern) 5. Missing Error ContextLocation: Issue: The error message doesn't indicate which project failed or provide debugging context. Recommendation: Include project name or model count in error messages. Severity: Low (better debugging experience) 6. Inconsistent NaN Handling DocumentationIssue: The design doc says "We treat NaN as 0 for discovery purposes and skip timestep 0 entirely," but the code skips step 0 AND treats NaN as 0. This creates potential confusion about whether NaN values can appear after step 0. Recommendation: Clarify in comments whether NaN can appear beyond step 0 and why. Severity: Low (documentation clarity) Minor Issues7. Code Style: Many ArgumentsLocation: The Severity: Very Low (code style preference) 8. Missing Const DocumentationLocation: Issue: Constants lack doc comments explaining their significance and tuning rationale. Severity: Very Low (documentation completeness) Questions
RecommendationsHigh Priority
Medium Priority
Low Priority
ConclusionThis is excellent work. The implementation faithfully follows the paper's algorithm, the test coverage is comprehensive, and the code is well-documented. The issues raised are mostly minor code quality improvements rather than correctness concerns. The most significant item is clarifying thread-safety expectations. Recommendation: Approve with minor changes requested. Great job on this complex algorithmic work! |
Code Review: Strongest-Path Loop Discovery AlgorithmI've completed a thorough review of PR #249. This is an impressive implementation of the strongest-path loop discovery algorithm from Eberlein & Schoenberg (2020). The code quality is generally high, but I've identified several areas for improvement. SummaryStrengths:
Areas for Improvement:
Critical Issues1. Inefficient HashMap Lookup in Loop Score ComputationLocation: src/simlin-engine/src/ltm_finding.rs:372-377 For each link in each loop at each timestep, the code performs a linear search through Recommendation: Pre-build a HashMap for O(1) lookups. High-Priority Issues2. best_score HashMap Pre-allocationLocation: src/simlin-engine/src/ltm_finding.rs:139-146 HashMap should be pre-allocated with capacity to avoid reallocations. 3. Missing Validation for Empty link_offsetsLocation: src/simlin-engine/src/ltm_finding.rs:302-305 Silently returning empty results could hide configuration errors. Add debug warnings. 4. parse_link_offsets Error HandlingLocation: src/simlin-engine/src/ltm_finding.rs:263 Silently ignores malformed variable names. Add debug assertions or warnings. Overall AssessmentThis is excellent work. The implementation is:
The identified issues are mostly minor optimizations. The code is production-ready after addressing the high-priority HashMap lookup performance issue. Code Quality Score: 9/10 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 75.48% 75.80% +0.31%
==========================================
Files 101 102 +1
Lines 22721 23066 +345
==========================================
+ Hits 17151 17485 +334
- Misses 5570 5581 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d02efdb393
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| for (var_name, &offset) in &results.offsets { | ||
| let name_str = var_name.as_str(); | ||
| if let Some(suffix) = name_str.strip_prefix(LINK_SCORE_PREFIX) { |
There was a problem hiding this comment.
parse_link_offsets only accepts variable names that start exactly with $⁚ltm⁚link_score⁚, but compiled results namespace submodel variables as <module>.<name> (see calc_flattened_offsets), so submodel link-score variables like sector.$⁚ltm⁚link_score⁚... are silently skipped. In projects that use module instances, this causes discover_loops to ignore those links entirely and return incomplete loop analysis.
Useful? React with 👍 / 👎.
src/simlin-engine/src/ltm_finding.rs
Outdated
| let main_model = project | ||
| .models | ||
| .values() | ||
| .find(|m| !m.implicit) |
There was a problem hiding this comment.
The causal graph is built from project.models.values().find(|m| !m.implicit), but project.models is a HashMap, so iteration order is not stable and this can pick different non-implicit models across runs. In multi-model projects, loop conversion/polarity can be computed against the wrong model definition, yielding inconsistent or incorrect discovery output; this should use the explicitly simulated model (e.g. main) instead of first-match iteration.
Useful? React with 👍 / 👎.
Use HashMap for O(1) link offset lookup in the hot scoring loop instead of linear scan per link per loop per timestep. Return a proper error when a link score variable is missing instead of silently treating it as NaN -- this indicates the project wasn't properly augmented with with_ltm_all_links(). Fix get_stock_variables to only examine the first non-implicit model, consistent with discover_loops() which builds the CausalGraph from the same model. Re-sort found loops by avg_abs_score descending after ID assignment so callers receive results ranked by importance. Tighten test assertions to exact counts and add subset validation for the heuristic discovery tests. Remove debug print/eprintln output from tests.
Code Review: Strongest-Path Loop Discovery AlgorithmThis PR implements a significant enhancement to the Loops That Matter (LTM) analysis, adding a heuristic-based loop discovery algorithm based on Eberlein & Schoenberg (2020). The implementation is thorough, well-tested, and follows good software engineering practices. Here's my detailed review: Strengths1. Excellent Documentation
2. Strong Test Coverage
3. Clean Architecture
4. Code Quality
Issues & RecommendationsCritical Issues1. Potential Performance Issue: O(V²) per Timestep (ltm_finding.rs:330-343)The current implementation runs the strongest-path search at every saved timestep (except t=0). For a model with 100 timesteps and 50 variables, this means 100 × O(V²) operations. for step in 1..step_count {
let graph = SearchGraph::from_results(results, step, &link_offsets, &stocks);
let paths = graph.find_strongest_loops();
// ...
}Recommendation: Consider adding a stride parameter to sample fewer timesteps, especially for long simulations. The design doc mentions this as a future optimization (line 24), but given the factorial growth the algorithm is meant to address, this could become a bottleneck. Suggested addition: pub fn discover_loops_with_stride(
results: &Results<f64>,
project: &Project,
stride: usize // Sample every Nth timestep
) -> Result<Vec<FoundLoop>>2. NaN Handling Inconsistency (ltm_finding.rs:89-90 vs 386-387)NaN is treated as 0 in // In from_edges (line 90):
let score = if score.is_nan() { 0.0 } else { score };
// But in score computation (line 386):
if value.is_nan() {
has_nan = true;
break;
}Recommendation: Document this behavior explicitly in the function documentation. If a link score is NaN (e.g., at t=0 or t=1 for flow-to-stock links), that loop probably shouldn't contribute to discovery at that timestep. Consider skipping timesteps where any link score is NaN during the search phase. Major Issues3. Missing Array Variable Check (ltm_finding.rs:307, ltm_augment.rs:87-101)The existing LTM code has Recommendation: Add explicit documentation in /// # Limitations
/// - Array variables are not supported (the project must not contain arrayed variables)
4. Memory Usage with Large Models (ltm_finding.rs:325-343)The algorithm accumulates all unique loop paths across all timesteps in memory ( Current: let mut all_paths: Vec<Vec<Ident<Canonical>>> = Vec::new();
let mut seen_sets: HashSet<Vec<String>> = HashSet::new();Recommendation: Consider using a bounded set with a priority queue to keep only the top N loops by score as they're discovered, rather than accumulating all unique loops first and then filtering. Minor Issues5. Hardcoded Constants (ltm_finding.rs:35-39)
Recommendation: Add optional parameters with defaults: pub struct DiscoveryConfig {
pub max_loops: usize,
pub min_contribution: f64,
pub timestep_stride: usize,
}
impl Default for DiscoveryConfig {
fn default() -> Self {
Self {
max_loops: 200,
min_contribution: 0.001,
timestep_stride: 1,
}
}
}6. Code Duplication in Link Offset Lookups (ltm_finding.rs:384-401)The loop score computation does a HashMap lookup for each link at each timestep. This is repeated work that could be optimized. Current: for step in 0..step_count {
for link in &links {
let link_key = (link.from.clone(), link.to.clone());
if let Some(&offset) = link_offset_map.get(&link_key) {
// ...
}
}
}Recommendation: Pre-compute link offsets once per loop outside the timestep iteration. 7. Unclear Variable Naming (ltm_finding.rs:420-423)
Recommendation: Add a comment: // Determine runtime polarity from actual loop scores (may differ from structural analysis
// if links have unexpected signs during simulation)
let runtime_scores: Vec<f64> = scores.iter().map(|(_, s)| *s).collect();8. Test Coverage Gap: Module LinksWhile Recommendation: Add an integration test in 9. Error Message Could Be More Helpful (ltm_finding.rs:395-401)When a link score variable is missing, the error mentions Recommendation: details: Some(format\!(
"Link score variable not found for {} -> {}. This usually means the project was not augmented with with_ltm_all_links() before simulation. Call project.with_ltm_all_links() before running the simulation, then pass those results to discover_loops().",
link.from.as_str(),
link.to.as_str()
)),10. Potential Precision Issue (ltm_finding.rs:376, 414-418)Using Current: let time = results.specs.start + results.specs.save_step * (step as f64);This is actually fine since it matches how Results stores time, but worth verifying against Security & Safety✅ No unsafe code Performance ConsiderationsAlgorithmic Complexity
Potential Bottlenecks
Recommendations:
Testing QualityExcellent Coverage
Missing Test Cases
Design Decisions to Validate1. Running at Every Saved Timestep (Design Doc Line 13)The doc acknowledges this is a simplification from the paper ("every computational interval"). Have you considered:
Suggestion: Document this tradeoff in the function documentation. 2. Absolute Values for Search (Design Doc Line 20)The decision to use 3. Deduplication by Node Set (ltm_finding.rs:244-246)This matches the existing Code Style & Conventions✅ Follows CLAUDE.md guidelines (commit message format, test-driven development) Minor Style Issues
Documentation Gaps
SummaryThis is high-quality work that demonstrates:
Recommended Actions Before MergeMust Fix (Correctness):
Should Fix (Quality):
Nice to Have (Future):
Overall AssessmentApprove with minor changes recommended. The implementation is sound, well-tested, and follows best practices. The identified issues are primarily about performance optimization and documentation clarity, not correctness. Great work on this implementation! The design document alone is a valuable artifact, and the code quality reflects careful attention to detail. 🎯 |
Use find_main_model() to look up "main" by canonical name instead of relying on HashMap iteration order, which is non-deterministic. Both get_stock_variables() and the CausalGraph construction now use the same deterministic lookup. Precompute results offsets per discovered loop path once, then use the offset vector in the per-timestep scoring loop. This avoids repeated HashMap lookups and Ident clones in the hot inner loop (previously O(L * T) HashMap lookups per loop, now just O(L) once). Rename test_self_loop_not_found to test_self_loop_found (the test asserts self-loops ARE found). Add recursion depth documentation on check_outbound_uses. Update design doc to say 7 unique loops for the arms race model (the two three-way loops share the same node set and are deduplicated).
Code Review - PR #249: Strongest-Path Loop Discovery AlgorithmExecutive SummaryThis PR implements a heuristic "loops that matter" (LTM) discovery algorithm based on Eberlein & Schoenberg (2020). The implementation is high quality with excellent test coverage, clear documentation, and strong adherence to Rust best practices. Recommendation: ✅ APPROVE with minor optional suggestions Strengths1. Excellent Code Quality
2. Algorithm Correctness
3. Comprehensive Testing
4. Outstanding Documentation
5. Clean Integration
Suggested Improvements (Optional)1. Redundant
|
Add unit tests for rank_and_filter covering MAX_LOOPS truncation, MIN_CONTRIBUTION filtering, and score-descending output ordering. Extract rank_and_filter() from discover_loops() to make these code paths directly testable. Fix test_parse_link_offsets to use canonicalize() directly instead of the canonicalize().to_source_repr() + from_unchecked() round-trip. The round-trip happens to work for LTM variable names (no dots) but is inconsistent with how the interpreter stores results keys. Remove nondeterministic fallback from find_main_model() -- it now only looks up "main" by canonical name, returning None if absent, instead of falling back to arbitrary HashMap iteration.
Code ReviewI've reviewed PR #249 implementing the strongest-path loop discovery algorithm. This is an excellent, well-implemented feature with comprehensive tests and documentation. Here's my detailed feedback: Strengths ✅1. Exceptional Documentation
2. Comprehensive Test Coverage
3. Performance Optimizations
4. Robust Error Handling
5. Code Quality
Suggestions for Improvement 🔧1. Recursion Depth Consideration ( The comment mentions that recursion depth is bounded by unique variables and that "very large models (1000+ variables) could in theory approach stack limits." Consider: // Add a depth limit to prevent stack overflow in pathological cases
const MAX_RECURSION_DEPTH: usize = 500;
fn check_outbound_uses(
&self,
variable: &Ident<Canonical>,
score: f64,
target: &Ident<Canonical>,
visiting: &mut HashSet<Ident<Canonical>>,
stack: &mut Vec<Ident<Canonical>>,
best_score: &mut HashMap<Ident<Canonical>, f64>,
found_loops: &mut Vec<Vec<Ident<Canonical>>>,
seen_sets: &mut HashSet<Vec<String>>,
depth: usize, // Add depth tracking
) {
if depth > MAX_RECURSION_DEPTH {
return; // Gracefully abort deep recursion
}
// ... existing checks ...
// When recursing:
self.check_outbound_uses(
&edge.to,
score * edge.score.abs(),
target,
visiting,
stack,
best_score,
found_loops,
seen_sets,
depth + 1, // Increment depth
);
}Alternatively, document why this is safe or add a debug assertion to catch unexpected cases during development. 2. Minor: Clone Efficiency ( While precomputing offsets is great, you still clone let offset = link_offset_map
.get(&(link.from.clone(), link.to.clone())) // Clones for lookupSince you're iterating over links from the CausalGraph, consider whether you can borrow here instead. This is minor since 3. Test Model Documentation The test models (
This helps future contributors understand the test suite. 4. Edge Case: Empty Results The code handles 5. Documentation: Algorithm Limitations The design doc mentions this is a heuristic and "NOT guaranteed to find the absolute strongest loop" ( /// Run the strongest-path loop discovery on simulation results.
///
/// This is a heuristic algorithm that finds important loops without exhaustive
/// enumeration. It is NOT guaranteed to find all loops or the absolute strongest
/// loops, but scales to large models where exhaustive enumeration is infeasible.
/// ...
Minor Style Notes 📝1. Consistent Error Messages Most error messages are excellent, but consider standardizing capitalization:
2. Test Assertions Some test assertions include the actual value in the message (good!), others don't:
This is already consistent in most places - excellent work! Security & Correctness ✅
Performance Considerations ⚡The optimizations in commits 3-5 are excellent:
One additional consideration: For very large simulations (many timesteps), you're building a
But this is likely premature optimization - the current approach is clear and correct. Conclusion 🎉This is production-ready code. The implementation is clean, well-tested, thoroughly documented, and follows all project conventions. The suggestions above are minor enhancements, not blockers. Recommendation: Approve and merge after considering whether to add:
Great work on a complex feature! The attention to detail in testing (especially the |
|
Addressed review feedback across 3 iterations: The main improvements are around correctness, performance, and test quality. The O(n) linear scan for link offset lookups in the hot scoring loop was replaced with a HashMap for O(1) access, and link offsets are now precomputed per discovered loop path to avoid repeated HashMap lookups and Ident clones in the per-timestep inner loop. Model selection was made deterministic by looking up "main" by canonical name instead of relying on HashMap iteration order. Missing link score variables now return a proper error instead of silently degrading to NaN. Test coverage was significantly tightened: integration test assertions now use exact counts with explanatory comments about heuristic behavior and validate that discovered loops are subsets of exhaustive results. New unit tests cover MAX_LOOPS truncation, MIN_CONTRIBUTION filtering, and score-based output ordering. Debug println/eprintln output was removed from tests, a misnamed test was corrected, and the design doc was updated to reflect 7 (not 8) unique loops in the arms race model after deduplication. |
1 participant
Summary
Test plan