Repository navigation
fix(plot): stop calling plotted points "samples"; correct the listing char count - #121
Merged
Merged
Conversation
Refs #120. The live readout said "5,296 samples" on a data-acquisition app, which reads as an acquisition total. It is not. The view-model appends at most one point per channel per render tick (20 Hz), so at 16 channels / 100 Hz the device delivers ~1,600 samples/s while this counted ~320 — about a fifth. Every "samples/s" figure measured during this release work was really this poll rate, which is why they all landed near 340 rather than 1,600. Now reads "N points plotted", and the property carries the arithmetic so the next reader does not have to rediscover it. Deliberately NOT fixing the underlying decimation in this commit. Counting the true total means incrementing in the shared streaming hot path (AbstractStreamingDevice, ~1,600/s, off the transport thread), and min/max decimation so transients survive is a larger plot change. Neither is a sensible risk days before a production flip, and both are already scoped in #120. This removes the false claim at zero risk to the data path. Recorded data was never affected: logging runs off the per-frame device message, not this poll, so a CSV export has always held every sample.
Removing the false "works with more than one device" bullet shortened the text, but the stated count was left at the pre-removal 1616. Verified against the actual block: 1572. The number matters because it is the only signal that the copy still fits Play's 4000-character limit without pasting it in to find out.
PR Summary by QodoFix live plot counter wording and update store listing character count
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Both plates are sized from measured text, with no bound. Making the
header label longer ("N points plotted" replacing "N samples") is what
surfaced it, but the legend plate has the same shape and the same risk —
its width comes from the widest channel label.
Nothing local sets ClipToBounds, so an oversize plate paints outside the
plot region rather than being cut off. Clamped both to the control width.
Realistically the header needs a very large count or a narrow layout to
overflow, which is why it had not been seen. That is an argument for
fixing it now rather than treating it as theoretical: the failure only
appears at the sample counts a long acquisition produces, which is the
case least likely to be caught by hand and most likely to be a user's
real session.
Member
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3e3418d |
cptkoolbeenz
added a commit
that referenced
this pull request
Aug 15, 2026
…label (#123) The committed set showed "N samples", which the app no longer says (#120, merged in e84df14). A store listing whose screenshots disagree with the running app is the kind of small wrongness a reviewer notices and a user trusts less. The hero shot is also simply better than what it replaces: three phase-staggered sines at a low enough sample depth to show three clean cycles, rather than the ten crammed ones the earlier capture had. It also exercises the plate-width clamp from #121 — the plate now hugs "201 points plotted" instead of running to a guessed width. Screens 03 and 04 are byte-identical to the previous set. That is expected, not a failed capture: the 90px crop removes the status bar with its clock, and neither the channel picker nor the scan result has other time-varying content. All four remain 1080x2160 — Play's 2:1 cap, and 1080 on the short side for promotion eligibility.
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
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.
Two display-accuracy corrections. No behaviour change to the data path.
The live plot called plotted points "samples"
Refs #120.
The readout said "5,296 samples" on a data-acquisition app, which reads as an acquisition total. It is not one.
MobileShellViewModel.PollActiveSamplesappends at most one point per channel per render tick (20 Hz), so at 16 channels / 100 Hz the device delivers ~1,600 samples/s while this counter sees ~320 — about a fifth.This also explains a number that ran through the whole release effort: every "samples/s" figure measured while validating the foreground service (~333–347/s) was really this poll rate, not the acquisition rate. Those measurements remain valid as a comparison — the same metric on both sides of the background test — but they were never the device's sample rate.
Now reads "N points plotted", and the property carries the arithmetic so the next reader does not have to re-derive it.
Deliberately not fixing the decimation here. Counting the true total means incrementing in the shared streaming hot path (
AbstractStreamingDevice, ~1,600/s, on Core's transport thread), and min/max decimation so short transients survive is a larger plot change. Neither is a sensible risk days before a production flip. Both stay scoped in #120.Recorded data was never affected. Logging runs off the per-frame device message (
DispatchDeviceMessage→LoggingManager), not off this poll, so a CSV export has always contained every sample. Only the live view is decimated.The listing copy's character count was stale
Removing the false "works with more than one device at a time" bullet (that claim is untrue —
MobileShellViewModelholds a single_connecteddevice) shortened the text, but the stated count stayed at the pre-removal 1616. Verified programmatically against the actual block: 1572. The number is the only signal that the copy still fits Play's 4000-character limit without pasting it in to find out.Follow-up, not in this PR
The four committed store screenshots still show the old "samples" wording. They need regenerating so the listing matches the app — the bench device is mid-soak for #116 right now, so that lands separately.