fix: clear Reliable/Unreliable consistency label with both metrics (fixes #149)#150
Conversation
The warning message always attributed inconsistency to CV>40% regardless of which condition actually failed. When consistency% was the trigger (e.g. CV=31.9% but only 62% of samples within ±1 SD), the message incorrectly implied CV exceeded the threshold. Apply Option B (issue #149) to both console output and PNG label: pattern-match (cv_failed, consistency_failed) and emit only the threshold(s) that were actually crossed: - CV only: "CV=42.3% (>40%)" - Consistency only: "Consistency=62% (<70%)" - Both: "CV=42.3% (>40%) and Consistency=62% (<70%)" - Neither: "insufficient samples (need >= 2)" Flagging logic in is_consistent() is unchanged; only diagnostic text fixed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the per-condition failure match with a single always-shown reliability line in both console output and PNG legend: Reliable: Consistency=82% (≥70%), CV=25.1% (≤40%) Unreliable: Consistency=69% (≥70%), CV=31.9% (≤40%) Changes: - Remove Consistency from the compact header line (now in reliability line) - Always emit both metrics regardless of which condition failed - Use ≥/≤ threshold notation for unambiguous pass/fail direction - Show CV=N/A when insufficient samples (coefficient_of_variation is None) - Drop TD_SAMPLES_MIN_FOR_STDDEV import from plot_step_response (unused) - Consistent plain-text prefix: no warning icon on either reliable or unreliable Closes #149. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughConsole output and Optimal P legend are refactored to report both Consistency and Coefficient of Variation metrics together with their respective thresholds, replacing the prior single-metric CV-only message with dual-metric reporting based on the is_consistent() flag. ChangesUnified Reliability and Consistency Reporting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plot_functions/plot_step_response.rs (1)
633-667: ⚡ Quick winConsider extracting duplicated reliability formatting logic.
This reliability formatting block (lines 635-649 specifically) is duplicated in
optimal_p_estimation.rslines 517-531. Both format CV and Consistency strings identically using the same constants and thresholds.Consider extracting this into a helper method on
TdStatistics, e.g.,format_reliability_metrics()that returns(cv_str, cons_str), to maintain DRY and ensure future changes stay synchronized.♻️ Suggested refactoring approach
Add to
TdStatisticsinoptimal_p_estimation.rs:impl TdStatistics { /// Format reliability metrics with thresholds for display pub fn format_reliability_metrics(&self) -> (String, String) { let cv_str = self.coefficient_of_variation.map_or_else( || "CV=N/A".to_string(), |cv| { format!( "CV={:.1}% (≤{:.0}%)", cv * 100.0, TD_COEFFICIENT_OF_VARIATION_MAX * 100.0, ) }, ); let cons_str = format!( "Consistency={:.0}% (≥{:.0}%)", self.consistency * 100.0, TD_CONSISTENCY_MIN_THRESHOLD * 100.0, ); (cv_str, cons_str) } }Then in this file, replace lines 635-649 with:
- let cv_str = analysis.td_stats.coefficient_of_variation.map_or_else( - || "CV=N/A".to_string(), - |cv| { - format!( - "CV={:.1}% (≤{:.0}%)", - cv * 100.0, - TD_COEFFICIENT_OF_VARIATION_MAX * 100.0, - ) - }, - ); - let cons_str = format!( - "Consistency={:.0}% (≥{:.0}%)", - analysis.td_stats.consistency * 100.0, - TD_CONSISTENCY_MIN_THRESHOLD * 100.0, - ); + let (cv_str, cons_str) = analysis.td_stats.format_reliability_metrics();Apply similar change in
optimal_p_estimation.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plot_functions/plot_step_response.rs` around lines 633 - 667, Extract the duplicated CV/Consistency formatting into a helper on TdStatistics (e.g., pub fn format_reliability_metrics(&self) -> (String, String)) that returns the cv_str and cons_str using TD_COEFFICIENT_OF_VARIATION_MAX and TD_CONSISTENCY_MIN_THRESHOLD; then replace the duplicated blocks in plot_step_response.rs (the block building cv_str/cons_str and cons_label) and optimal_p_estimation.rs to call TdStatistics::format_reliability_metrics(), keep the existing is_consistent() check to select label text ("Reliable"/"Unreliable") and color (COLOR_OPTIMAL_P_TEXT/COLOR_OPTIMAL_P_WARNING), and push the same PlotSeries as before. Ensure the new helper is added where TdStatistics is defined so both modules can use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/plot_functions/plot_step_response.rs`:
- Around line 633-667: Extract the duplicated CV/Consistency formatting into a
helper on TdStatistics (e.g., pub fn format_reliability_metrics(&self) ->
(String, String)) that returns the cv_str and cons_str using
TD_COEFFICIENT_OF_VARIATION_MAX and TD_CONSISTENCY_MIN_THRESHOLD; then replace
the duplicated blocks in plot_step_response.rs (the block building
cv_str/cons_str and cons_label) and optimal_p_estimation.rs to call
TdStatistics::format_reliability_metrics(), keep the existing is_consistent()
check to select label text ("Reliable"/"Unreliable") and color
(COLOR_OPTIMAL_P_TEXT/COLOR_OPTIMAL_P_WARNING), and push the same PlotSeries as
before. Ensure the new helper is added where TdStatistics is defined so both
modules can use it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad9e54e2-7cce-4add-9d4b-95d567815a37
📒 Files selected for processing (2)
src/data_analysis/optimal_p_estimation.rssrc/plot_functions/plot_step_response.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All constants go insrc/constants.rs— no hardcoded magic numbers in function code
Axis indices (0=Roll, 1=Pitch, 2=Yaw) must use thesrc/axis_names.rsmodule withAXIS_COUNT,AXIS_NAMES, andaxis_name()function instead of creating redundant axis index constants
ALWAYS useFirmware revisionheader to detect firmware type, NEVER useFirmware type
Firmware revision patterns: Betaflight usesBetaflight 4.x.xorBetaflight YYYY.MM, Emuflight usesEmuFlight 0.x.x, INAV usesINAV x.x.x
Files:
src/plot_functions/plot_step_response.rssrc/data_analysis/optimal_p_estimation.rs
🔇 Additional comments (3)
src/plot_functions/plot_step_response.rs (1)
15-15: LGTM!src/data_analysis/optimal_p_estimation.rs (2)
506-506: LGTM!
515-537: LGTM!
Artifacts: https://github.com/nerdCopter/BlackBox_CSV_Render/actions/runs/26253231908
Fixes #149 — the consistency warning in Optimal P Estimation always blamed
CV>40%even when the actual failure wasConsistency<70%, causing confusing messages likeCV=31.9% — unreliable (>40%).Reliable/Unreliablelabel followed byConsistency=XX% (≥70%), CV=XX% (≤40%), regardless of which condition failed≥/≤makes the pass/fail direction self-evident for both metricsCV=N/Awhen insufficient samples (coefficient_of_variationisNone)Consistency=XX%from the compact header line — no longer duplicatedReliable:/Unreliable:prefix, consistent in both statesBefore
After
Test plan
cargo fmtapplied./input/*HELIO*files with--step --estimate-optimal-p20260103_160240.01Roll —Consistency=69% (≥70%), CV=31.9% (≤40%)20260503_140158_master.01Roll —Consistency=90% (≥70%), CV=90.1% (≤40%)20260503_112224_PR1139_hover-testRoll —Consistency=54% (≥70%), CV=49.3% (≤40%)🤖 Generated with Claude Code