Skip to content

MAVExplorer: Add histrograms - #1681

Open
stephendade wants to merge 1 commit into
ArduPilot:masterfrom
stephendade:histro
Open

MAVExplorer: Add histrograms#1681
stephendade wants to merge 1 commit into
ArduPilot:masterfrom
stephendade:histro

Conversation

@stephendade

Copy link
Copy Markdown
Contributor

This PR adds histrogram-style graphs to MAVExplorer. This is useful for analysing the performance (such as crosstrack) of a vehicle.

Can be used similarly to graphs:
histrogram NTUN.XTrack

The histrogram will autoupdate it's range based on panning or zooming on other graphs.

There are 2 options:
-hist_bins for specifying the number of histrograms bins (default 50)
-hist_show_stats for overlaying the median and std dev (default true)

Screenshot from 2026-04-27 18-11-07

Co-authored-by: Copilot <copilot@github.com>
@tridge

tridge commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/2026_09_01_AIReview/devcall_pr_reviews.html#prMAVProxy-1681

Reviewed at head ea41a2ea94. REQUEST CHANGES — the feature is purely additive and the process model is right; the verdict rests on two defects I reproduced, both of which make it misbehave silently rather than fail visibly.

1. A single NaN turns every overlaid statistic into NaN, and the plot still looks fine. np.median/np.mean/np.std are called without filtering, so one NaN gives median nan mean nan std nan, axvline(x=nan) draws nothing, and the legend reads "Median nan". Meanwhile matplotlib silently drops NaN inside hist(), so the bars render normally — the user gets a plausible histogram with the statistics quietly gone. Reproduced: 1000 samples + one NaN → all three stats NaN; with y = y[np.isfinite(y)] → median 0.005, mean 0.021, std 1.004. NaN is common in MAVExplorer expressions (EKF innovations, sqrt/asin out of range). One filter line at the top of _draw_histogram fixes it — and it also fixes the weighting, which currently counts NaNs in the denominator that hist then discards (bars summed to 99.9%).

2. Bad bins or non-finite data kill the child process with no message anywhere. set hist_bins 0 and histogram --bins 0 are both accepted (the MPSetting has no range=), and since plotting happens in a spawned child the exception is invisible in MAVExplorer's console — no window, no error. All three reproduced against the exact ax.hist(..., weights=...) call: bins=0ValueError: bins must be positive, when an integer; an inf in the data → supplied range of [...] is not finite; an all-NaN field → autodetected range of [nan, nan] is not finite.

Also worth fixing: "field has no samples" is only print()ed inside the child, so a GUI session shows nothing — and the parent already knows the series is empty before it spawns. A time-range selection with zero samples assigns self.xlim before the empty-guard returns, leaving the previous full-range histogram on screen looking current while everything else is zoomed elsewhere. Quoted expressions containing spaces are split into separate fields with only index zero plotted. The Tools > Histogram menu item dispatches with no arguments and is a no-op. And the completion rule and help string both advertise multiple fields while the command explicitly rejects them.

Checked and correct, so nobody re-litigates it: the process model is right — everything wx/pyplot-touching runs only in the child, pipe-end ownership matches Graph_UI/MavGraph exactly, and check_xlim_change returning None is correct here because the histogram's x-axis is a value axis that must not be broadcast as a time range. No figure leak. And the "histrogram" misspelling in the title did not reach any code, command name or user-visible string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants