feat: Optimal P source provenance and unified console/PNG label format#151
Conversation
…ptimal P
Add source_events and source_files to OptimalPAnalysis, populated from
AircraftProfile.file_count and AxisProfile.event_count after analysis.
Console and PNG labels are now identical in content and order:
- Td line: includes tolerance (±) and Noise level
- Deviation: separate indented line with sign and zone name
- Source: Group/Single — N flight(s), M throttle-punch(es)
- Current P=N
- Recommendation: unified format for all four arms
(Conservative / Decrease / Optimal / Investigate)
- Reliable/Unreliable: moved after Recommendation; uses ⊢≥/⊢≤ notation
All lines under "Optimal P (Experimental, log-derived)" are 2-space indented.
PNG no longer says "See console output for details" for Investigate arm.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR extends the optimal P analysis with provenance tracking. ChangesProvenance Tracking and Reporting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/data_analysis/optimal_p_estimation.rs`:
- Around line 509-518: The console label generation using output.push_str(...)
emits an axis-prefixed Td line but omits the "Optimal P (Experimental,
log-derived)" header, causing structural mismatch with the PNG block; update the
formatter that builds the label (the code using output.push_str and the symbols
axis_name, self.td_stats, self.td_target_ms, self.td_tolerance_ms,
self.td_stats.num_samples, self.noise_level.name()) so it first emits the
"Optimal P (Experimental, log-derived)" header (matching the PNG block) and then
emits per-axis Td/Noise lines in the same order/structure as the PNG output,
ensuring verbatim parity between console and PNG content.
In `@src/main.rs`:
- Around line 515-517: The profile.file_count is being set to files_profiled
which counts any parsable file, overstating flights; change this to count only
files that actually contributed throttle-punch events (e.g., introduce or use an
existing counter like files_with_contribs or compute from the collection of
per-file event counts) and assign that value to profile.file_count instead of
files_profiled; ensure the increment of that counter happens only when a file
produces a non-zero number of throttle-punch events (update the logic that
currently increments files_profiled to increment the new counter only on
contribution).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7a850b5-5107-4625-b311-7e2e5f163b78
📒 Files selected for processing (4)
src/data_analysis/optimal_p_estimation.rssrc/data_analysis/torque_inertia_profiler.rssrc/main.rssrc/plot_functions/plot_step_response.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Lint
🧰 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/data_analysis/torque_inertia_profiler.rssrc/plot_functions/plot_step_response.rssrc/data_analysis/optimal_p_estimation.rssrc/main.rs
🔇 Additional comments (3)
src/data_analysis/torque_inertia_profiler.rs (1)
116-117: LGTM!Also applies to: 130-130
src/main.rs (1)
1252-1255: LGTM!src/plot_functions/plot_step_response.rs (1)
590-590: LGTM!Also applies to: 594-604, 609-622, 627-649, 651-666, 671-703, 706-740
- Rename 'Source:' → 'Td source:' (lowercase, clarifies it is the Td physics-target's provenance, not the step-response measurements) - Move 'Td source:' line before 'Deviation:' (matches user-specified order) - Add 'Td target: physics-derived...' preamble to PNG legend for parity with console header; parameterize 'log group' vs 'log file' by file_count - Rename Group/Single labels to 'File Group' / 'Single File' - Console header also parameterized: 'in log group' vs 'in log file' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use plain 'flights' and 'throttle-punches' — always plural in practice. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
'Td source: File Group — N flights, M throttle-punches' already conveys the same information. The preamble line is only needed in the console section header, not repeated in every axis's PNG legend. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Noise was incorrectly packed onto the Td header line. Restore it to its own ' Noise: LEVEL' line as it was in master. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Noise is a per-flight measurement; placing it after Td source groups context lines (source + noise) before result lines (deviation, recommendation). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… changes - Expand Output section into a structured list showing each label line (Td, Td source, Noise, Deviation, Current P, Recommendation, Reliable/Unreliable) with descriptions - Update Reliability section: 'CV warning' → 'Unreliable:' label, 'CV = None' → 'CV = N/A', note that Consistent/CV are independent of source flight/punch count - Add Td source and Reliable/Unreliable to the dependability signals summary Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…P PNG - Console base P:D: compute is_low_authority from valid_window_max_setpoints (already available in scope); print [LOW AUTHORITY] warning; replace Moderate and Aggressive prints with 'Skipped [LOW AUTHORITY]' - PNG base P:D: gate Moderate block behind !is_low_authority; show 'Recommendation (moderate): Skipped [LOW AUTHORITY]' otherwise (Aggressive is nested inside Moderate so is implicitly gated too) - PNG Optimal P section: add [LOW AUTHORITY] label after section header when is_low_authority fires Conservative is retained in all cases — a small incremental step is safe regardless of measurement precision. Only flights with max setpoint below LOW_AUTHORITY_SETPOINT_THRESHOLD_DEG_S (100 dps) are affected. Closes #152 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion Introduce SetpointAuthority enum (Low/Moderate/High) and compute_setpoint_authority() in calc_step_response.rs, using the **mean** of per-window max setpoints rather than max, so a single outlier input no longer masks a predominantly hover-style flight. Changes: - constants.rs: add HIGH_AUTHORITY_SETPOINT_THRESHOLD_DEG_S (250 dps), update LOW_AUTHORITY comment to describe all three tiers - calc_step_response.rs: add SetpointAuthority enum with name()/is_low() and compute_setpoint_authority() returning Option<(level, mean)> - main.rs / plot_step_response.rs: replace is_low_authority binary check with compute_setpoint_authority(); always show all recommendations (no more "Skipped — LOW AUTHORITY"); emit always-visible "Setpoint Authority: LEVEL (mean=Xdps ⊢≥100dps)" line (orange for LOW) PNG legend refinements: - "step-response" → "Step-Response Avg", "Peak:" → "Smoothed Peak:" on curve labels; "Peak=" → "Actual Peak=" on Current P:D line - ─────────────────────── separator (consistent with Optimal P) between curve entries and P:D section; 2-space indent on all lines below Current P:D Console alignment with PNG: - "Peak=" → "Actual Peak="; recommendation format uses () not →; (P=N) suffix removed from all recommendation lines; blank line added after axis header Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…W.md - plot_step_response.rs: replace RGBColor(80, 80, 80) magic number with the already-imported COLOR_OPTIMAL_P_TEXT constant for the non-LOW Setpoint Authority label color - OVERVIEW.md: replace outdated [LOW AUTHORITY] binary-flag description with Setpoint Authority classification (LOW/MODERATE/HIGH), update dependability signals table and CV section to match new terminology Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…header - profile_aircraft_group: increment files_profiled only when total_events > 0, so file_count (and the Td source provenance line) excludes logs that were parsed successfully but produced zero throttle-punch events - Rename console section header from '--- Optimal P Estimation ---' to '--- Optimal P (Experimental, log-derived) ---' to match the PNG legend header text Addresses CodeRabbit review comments on PR #151. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Builds on the Reliable/Unreliable label fix from #150 (fixes #149) by adding source provenance, Setpoint Authority classification, and fully aligning console and PNG label layout.
Optimal P source provenance
source_eventsandsource_filesfields toOptimalPAnalysis, populated fromAircraftProfile.file_countandAxisProfile.event_countTd source:line:File Groupwhen data pooled across ≥2 files;Single Filefor single-file runsTd target: physics-derived from throttle-punch events in log group|file.) printed once per aircraft in console; removed as a separate PNG legend line (redundant withTd source:)Setpoint Authority classification
[LOW AUTHORITY]flag with always-visibleSetpoint Authority:line (console + PNG)SetpointAuthorityenum (Low/Moderate/High) andcompute_setpoint_authority()incalc_step_response.rsLOW < 100 dps,MODERATE 100–250 dps,HIGH > 250 dps(constants inconstants.rs)LOW, neutral forMODERATE/HIGHLabel line order — console and PNG (identical content)
PNG legend refinements
Step-Response Avg (Smoothed Peak: X.XX, Td: Xms);< N deg/s (Smoothed Peak: …)/≥ N deg/s (Smoothed Peak: …)─────────────────────separator between curve entries and P:D section (consistent with Optimal P divider)Current P:D=…Console/PNG alignment
Peak=→Actual Peak=(matches PNG); recommendation format uses()not→;(P=N)suffix removedTest plan
cargo fmtno-opSingle File — 1 flight, N throttle-punchesFile Group — N flights, M throttle-punchesSetpoint Authority: LOW (mean=Xdps ⊢≥100dps)shown in orange; all recommendations still visibleSetpoint Authority: MODERATEorHIGHin neutral colorSmoothed Peakon curve labels;Actual PeakonCurrent P:Dassessment line─────────────────────separator visible between curve entries and P:D block🤖 Generated with Claude Code