Repository navigation
feat(cw): colour DeepFist letters by the model's per-letter confidence (#6099). Principle VIII. - #6103
Conversation
…r CW is not gated out (aethersdr#5950). Principle XI. The stream default (12) comes from n9bc/DeepFist tools/squelch.py, calibrated between dead air and a tuned-in signal; weak off-air CW scores in that gap and the gate dropped most of it. The app's parameters now set 3; carriers stay held off by the completed-mark guard (the carrier regression now also runs at the app's parameters). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
aethersdr#6099). Principle VIII. Each letter carries the mean posterior over its frames, averaged over the windows that saw it before it settles; the panel colours it with the four existing cost bands and never applies the Sens threshold. ggmorse is unchanged. Refs aethersdr#6099 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…). Principle VIII. Refs aethersdr#6099 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes, for #6099's three code criteria. Each DeepFist letter now reaches the panel as its own (text, cost) piece and is coloured by the existing four bands; the Sens threshold is not applied to it; ggmorse's path is untouched apart from the band table moving into cwCostColor() (I compared the boundaries — 0.15 / 0.35 / 0.60 and the four literals are identical, so the ggmorse refactor is behaviour-preserving). The fourth criterion ("colour table reproduced on a recording not used to choose the method") is evidence I can read but not reproduce: the two held-out W1AW rows are in the body and the bundle, and I take them as stated.
One criterion deserves a note rather than a tick: #6099 asks for "a socket-free test [that] pins the per-letter score". The committer's cross-window average is pinned (and pinned well — see "what I tried to break"); spanConfidence(), the function that actually computes the score, has no test at all. Details in the nits.
Scope
| File / group | What it changes | Claimed by | Verdict |
|---|---|---|---|
DeepFistCommitter.h |
Token::confidence, Piece, sighting history + mean |
#6099 | In scope |
DeepFistStream.{h,cpp} |
spanConfidence(), optional pieces out-param |
#6099 | In scope |
CwRxModel.{h,cpp} |
coloredTextDecoded, DeepFist backend rewired to it |
#6099 | In scope |
DeepFistCwModel.h/.cpp — scoredTextDecoded, pieces in run() |
per-piece emit | #6099 | In scope |
DeepFistCwModel.h/.cpp — appParameters(), activityThreshold = 3 |
the #5950 threshold | #5953 (bf9e0338) |
Stacked base — not this PR's; review under #5953 |
MainWindow.h, MainWindow_DigitalModes.cpp |
new slot + UniqueConnection |
#6099 | In scope |
PanadapterApplet.{h,cpp} |
cwCostColor() extraction, appendColoredCwText() |
#6099 | In scope |
tests/deepfist_committer_test.cpp |
confidence cases | #6099 | In scope |
tests/deepfist_cw_model_test.cpp, tests/tests.cmake |
weakSignal(), carrier test extension, deepfist_weak_signal_test |
#5953 | Stacked base |
docs/deepfist-cw-backend.md |
threshold sentence (#5953) + confidence/colour sentence (#6099) | both | Mixed, both disclosed |
Nothing in the diff is unexplained, and CHANGELOG.md is correctly untouched. The stacked rows are disclosed in the first line of the body with the base SHA, which is the right way to do it — I did not review them on their merits. The merge-order consequence is worth stating plainly for the maintainer: #5953 is still open and assigned, so merging #6103 first lands the threshold change through this PR rather than through its own review. And every colour-band figure in #6099 and in this body was measured at threshold 3, so if #5953 lands at a different value the band calibration needs re-measuring.
No new settings keys, no capability fields, no protocol surface, no new thread, no new colour literal. New public surface is three signals and two static helpers, all C++-internal.
Blockers
None. I could not find a case where this produces wrong text, loses text, crashes, or changes ggmorse behaviour.
Nits (non-blocking)
- Light-theme contrast: DeepFist text goes from 13.4:1 to 1.23:1 (inline on
PanadapterApplet.cpp:931). The old unscored path usedcolor.text.primary; the new path uses the ggmorse literals. Resolving the tokens inresources/themes/default-light.json(color.background.0→gray.900→#f5f5f8) against the four literals gives 1.23 : 1 green, 1.29 yellow, 2.08 orange, 3.18 red — and by this PR's own tables ~85 % of letters are green. On the dark theme all four are fine (14.2 / 13.5 / 8.4 / 5.5). ggmorse already has this, so the change is consistency rather than a new class of defect, anddocs/a11y.mdexplicitly permits concrete hex as "a valid intermediate state — file a follow-up issue to add the token". The body says mapping the bands to tokens is "left for a follow-up" with no issue cited. Please file it and reference it here; and consider saying in the body that DeepFist text loses its theme token, since right now that reads as a neutral detail. - Two different tolerances for "the same letter in an earlier window" (inline on
DeepFistCommitter.h:33): the sighting lookup uses 0.12 s, the pending matcher just below uses 0.16 s. A letter whose time estimate drifts 0.12–0.16 s between windows is still carried as the same pending character but silently loses its earlier sightings, falling back to the single-window value — i.e. to the behaviour the mutation test is designed to catch, for exactly the jittery weak copy the feature exists for. Also, unlike the pending matcher ("one-to-one matching preserves repeated characters as separate events"), the sighting scan is not one-to-one, so a fast same-letter pair can take the other letter's sighting. Same-id-within-0.12 s is the one case where the two matchers diverge and the test does not cover it. spanConfidence()has no direct test (inline onDeepFistStream.h:55). It is static, pure and deterministic — the ideal socket-free CTest subject — butDeepFistStream.cpponly compiles underENABLE_DEEPFIST_EXPERIMENT, which no workflow sets, so today it is unreachable from any CI-built target. Making it header-only (it touches no model type) would put it ondeepfist_committer_test's lane, which is the existing precedent for "model-free algorithms: no sockets, weights or ORT".Token::confidencedefaults to1.f, so a producer that forgets to set it gets green rather than red. OnlyDeepFistStreamand the tests construct Tokens today, so nothing is wrong now;0.fwould fail safe.
Socket tests
No socket test added, modified or removed; no QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost and no Fake* peer in either changed test file. deepfist_committer_test links Qt6::Core only; deepfist_weak_signal_test (stacked, #5953) runs the vendored model over constructed audio and skips with 77 without a model.
What I tried to break
- The committer arithmetic, by hand, window by window.
Apublishes at 0.6 = mean(0.9, 0.6, 0.3) — the current sighting viacurrent=trueplus two from history, with this window's tokens pushed after the publish loop so there is no double count;Tcarries through the gated tick at 0.4 = mean(0.2, 0.4, 0.6) withcurrent=false, its own pending confidence covered by the third history entry rather than added twice. The mutation the body claims is real: current-window-only gives 0.3 and exits 22. The test is not asserting the implementation back to itself. - Does
spanConfidence()measure what its comment says? This was my best shot at a real finding and it held.greedyCtcFrames()(third_party/deepfist/DeepFistCtc.cpp:43-57) pusheston the first frame of each collapsed run, so walking forward fromframes[j]really is the letter's run, not a single frame. AndDeepFistStreamcalls it with the defaultblankPenalty = 0.0f, so the decoder's argmax andspanConfidence()'s plainmax_elementare the same argmax — had the penalty been non-zero the span would truncate on exactly the weak letters this feature is for. The logits-vs-log-probs invariance claim also checks out algebraically (softmax(log p) = pwhenpis normalised). - Text/colour divergence.
publish()andseparate()are the only writers to bothoutputandpieces, so the concatenated pieces equal the returned string;piecesis scoped per queue item alongsideoutput; onstream.failed()the worker returns before emitting, so a mid-item inference failure drops both together.spanConfidence()cannot divide by zero (thebreakis guarded byt > frame, socount >= 1), and out-of-range input returns0.f→ red rather than a bad read. NaN logits would give NaN cost, which falls through every band to red — fail-safe. - Duplicate append.
DeepFistCwModelstill emits the whole-batchtextDecoded(output)and the per-piece signal, which looked like double text in the panel — but after this PRDeepFistCwModel::textDecodedhas no consumer outsidetests/deepfist_cw_model_test.cpp; the backend connectsscoredTextDecodedonly. No double insert.unscoredTextDecodedis likewise producerless now, which the body discloses. - Lifecycle. The new connection uses the same
generation == m_generation && isRunning()guard inCwRxModel::bind()as its two siblings,Qt::UniqueConnectioninrefreshCwRxBackend()makes repeated backend switches idempotent, andm_historydies with theDeepFistStreamthat is rebuilt on every generation change inrun(). History is bounded: 8 s of pruning attickSamples = 1280(0.4 s/tick) is ~20 windows, and the per-letter scan is O(history × ticks) on a handful of letters per second. - Governance.
GOVERNANCE.md:97makes "any change to visual design — colors, fonts, spacing, theme, icons" RFC-gated, and:221says an agent "may not autonomously change" it, with:226prescribing exactly what this PR did: implement and note that a design decision needs maintainer review. #6099 already carriesmaintainer-review. I am not calling this a violation — the required disclosure is present — but it is the maintainer's call, not mine. - Could not check: no build and no GUI here, so every finding above is reasoned from the code rather than reproduced at runtime, and the replay/CER tables are taken as stated. Also worth being blunt about CI: the five green checks prove the default build still compiles with the unconditional
coloredTextDecodedsignal and thecwCostColor()refactor — no workflow setsENABLE_DEEPFIST_EXPERIMENT, soDeepFistStream.cpp,DeepFistCwModel.cppandappendColoredCwText()were not compiled or run by any check on this head.deepfist_committer_testis registered unconditionally (tests/tests.cmake:7640-7644), so the new confidence cases do build and run in the full suite, though not on the frozen per-PR gate — the body says both of these accurately.
Recommendation
Needs maintainer decision. The engineering is clean and unusually well evidenced, I found no blocker, and the implementation seams match the triage sketch. Two things are above my pay grade: the PR is stacked on an undecided #5953, so the merge order decides where the threshold change gets reviewed; and GOVERNANCE.md:97 puts colour assignment with the maintainer, which the author correctly flagged rather than assumed. Concrete next step for @skerker, both small and independent of those calls: file the theme-token follow-up issue docs/a11y.md asks for and cite it here (noting the light-theme ratios), and either align the 0.12 s sighting window with the matcher's 0.16 s or add a sentence saying why they differ.
Thanks for the held-out-recording table and the mutation log — being able to check which rows chose the method made this much faster to review.
🤖 aethersdr-agent · cost: $7.0666 · model: claude-opus-5
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
Partially. Head 4c9a73e57374f8344234cacf1d6281a6c20474a2 implements #6099's per-letter confidence path, keeps the sensitivity filter out of DeepFist, and leaves the ggmorse publication/spotting route separate. However, the cross-window matcher can assign one repeated letter another letter's confidence. The new foreground mapping also needs the light-theme regression below addressed.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/core/deepfist/DeepFistStream.{h,cpp} |
Per-span posterior, optional scored pieces | Yes | In scope |
src/core/deepfist/DeepFistCommitter.h |
Sighting history and per-letter averaged confidence | Yes | In scope |
src/models/DeepFistCwModel.{h,cpp}, CwRxModel.{h,cpp} |
Scored publication signals and generation guard | Yes | In scope |
src/gui/MainWindow.h, MainWindow_DigitalModes.cpp, PanadapterApplet.{h,cpp} |
Route scored pieces and select existing bands | Yes | In scope |
tests/deepfist_committer_test.cpp |
Cross-window/carried confidence arithmetic | Yes | In scope |
DeepFistCwModel::appParameters(), tests/deepfist_cw_model_test.cpp, tests/tests.cmake |
Threshold 3 and weak/carrier coverage inherited from #5953 | Explicitly disclosed stack | Review/resolve #5953 before landing its behavior through this PR |
docs/deepfist-cw-backend.md |
Threshold and scored-output contract | Yes | Both parts disclosed |
The older authored threshold commit is the disclosed stack, not hidden bundling. The two feature commits and merge commit are explained. No new dependency, settings document, protocol verb, thread, or CHANGELOG edit. The new signals are internal C++ surface. Scope/checklist claims hold, subject to the stacked dependency and the visual design decision explicitly noted in the body.
Blockers
- Match repeated letters to their own earlier sightings.
src/core/deepfist/DeepFistCommitter.h:33-38takes the first reverse-ordered same-ID sighting within 0.12 seconds from each earlier window. That can be the following letter, even when an exact-time match exists. A focused executable using this exact header, with both windows containingEat 2.80 s/confidence 0.9 and anotherEat 2.88 s/confidence 0.1, returned:
E confidence 0.500
E confidence 0.100
Both sightings of the first letter are 0.9, so its mean should be 0.9 (green); it is incorrectly orange because the later E's 0.1 was borrowed. Closely spaced repeated letters are representable by CTC and occur in fast machine-sent CW; the timestamp tolerance is larger than their separation. This is an executed committer-level reproduction, not a claim that the demo produced that sequence. Select the closest matching token per earlier window and preserve one-to-one matching for repeated letters; add a socket-free regression with two nearby equal IDs having distinct scores. The existing test only spaces its equal-ID text cases farther apart and does not catch this.
- Keep the newly coloured DeepFist text readable in Light themes.
src/gui/PanadapterApplet.cpp:931now applies the literalcwCostColor()table to DeepFist output. PreviouslyappendUnscoredCwText()usedThemeManager::color("color.text.primary"). In an exact-head full build, I selected Default Light, connected only DEMO-0001, selected DeepFist and enabled the simulator's CW tone with noise/carrier disabled. The decoder reached “DeepFist ready” and produced green letters; the actualcwDecodePanelcapture shows very faint text on the light background. The new green#00ff88against Default Light's#f5f5f8has only 1.23:1 contrast (old primary text#1a2a3a: 13.44:1). Yellow is 1.30:1; orange 2.08:1. This is a regression for DeepFist users of the shipped Light theme even though the table already existed for ggmorse. Resolve the bands through theme tokens with readable Light values and verify both themes; reusing the pre-existing table does not preserve the former DeepFist readability.
Nits / maintainer decisions (non-blocking apart from the defects above)
src/gui/PanadapterApplet.cpp:825-835: Jeremy, the move to confidence colouring and its band definitions is the visual-design decision #6099 and the PR body defer to you. Reusing a literal table does not make newly coloured output theme-aware. Keep #5953's activity-threshold decision separate; all supplied confidence evidence is at threshold 3.tests/deepfist_committer_test.cpp:95-110covers sighting arithmetic, but does not directly exerciseDeepFistStream::spanConfidence(). I independently compiled that production function and verified posterior averaging, termination when argmax changes, null input, and invariance under adding 1000 to all logits. Keeping that small deterministic coverage in the registered suite would protect the score extraction as well as the aggregation.tools/score_pieces.py:83-95in the attached evidence bundle removes spaces and normalizes each piece separately; itsnorm(text)does not usehyp=True. The older #5950 scorer normalizes the complete hypothesis withhyp=True, including removal of the station-ID phrase. Thus the PR's “same scorer used in #5950/#5953” wording needs qualification: shared alignment/normalization helpers do not make these CER definitions identical. The per-colour percentages do reproduce from the bundled labelled CSVs; this is a provenance/wording issue, not a refutation of their measured correlation.
What I verified and tried to break
- The exact head's
deepfist_committer_testpassed. Removing history from the averaging calculation in a separate scratch header made it exit 22, independently reproducing the author's mutation claim. Text carry, expiry, delayed separators, and its existing repeat cases survived. - The independent nearby-repeat probe above exposes the missing association case. A separate executable compiled the actual
spanConfidence()source; results were approximately 0.7 before and after a +1000-logit shift. No synthetic firmware or socket-owning test was introduced or run. - Checked the ordinary and scored model paths, stop/switch generation checks, and GUI routing. ggmorse's four boundaries and colours are byte-for-byte the old table. The scored append has no sensitivity suppression and does not emit the callsign-spotting signal. MQTT's existing route remains unchanged by the diff.
- Downloaded and inspected the supplied evidence bundle without executing its scripts. Independently counted the four labelled CSVs: all stated per-colour counts and percentages agree. The absent original audio means those replay tables were not re-inferred locally. The above scoring qualification is based on reading the supplied scorer.
- Engine-boundary strict checks on isolated merge-base and head snapshots both exited 0 with identical complete output; manifest and tag files are unchanged. No new above-seam include or enforcement baseline was added.
The exact head also completed a full Linux Release build with DeepFist enabled and the pinned ONNX runtime/model present. Its isolated offscreen automation session used a separate settings directory/socket and connected exclusively to serial DEMO-0001 (model AetherSDR Demo, transmit false). The actual model reached ready and decoded simulated CW through the production publication/GUI path. I captured and inspected the Light-theme decode panel. This proves the rendering regression; it is not an over-the-air accuracy benchmark, a full-suite run, or a before/after binary comparison. The earlier renderer's contrast comparison above is calculated from its retained production renderer and shipped theme values.
The earliest CI runs for this head were created 2026-10-02T22:46:54Z; current main's tip timestamp is 2026-10-04T18:26:40Z. Green checks therefore cover an older merge result, and non-strict protection does not force a refresh. Hosted workflows do not enable DeepFist; their green build is not proof of the experimental code path or the full-suite claim. No separate code-review skill was available; this was a manual audit with compiled probes.
Recommendation
Request changes. Correct the per-letter association and theme regression, add focused coverage, and resolve the disclosed stacked/default/visual decisions before landing. Refresh the stale merge checks after remediation.
— Jeremy KK7GWY + Codex
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
Partially. Head 4c9a73e57374f8344234cacf1d6281a6c20474a2 delivers #6099's mechanism:
- DeepFist publishes
(text, cost)pieces. - The panel colours them with the four existing bands.
- The Sens filter never applies to them.
- ggmorse's path is unchanged (same four boundaries and literals, now in
cwCostColor()).
Two defects stop it short of "each letter coloured by its own confidence, readably". Close repeated letters can swap scores (Blocker 1, reproduced). In the Light theme, the colours that replace DeepFist's theme token are below the contrast ratio canon requires (Blocker 2). I re-verified both independently of the previous ten9876 review, which found the same two.
Stacking: this PR carries #5953
This branch is #5953's commit bf9e0338 byte-for-byte, then a merge of main, then this PR's own 41a889ef and 4c9a73e5.
- #5953 must be decided first. Merging this PR first lands the threshold change through this review instead of #5953's.
- If 3 is accepted, a later merge of
mainhere is clean, since the content is identical. - If 3 is changed or rejected,
bf9e0338has to come off this branch, and the colour-band calibration (all measured at threshold 3) needs re-measuring. - The body still says this PR "stays a draft until #5953 is decided". It was marked ready for review on 2026-10-02 23:15Z with #5953 still undecided. Please update the body, or move it back to draft.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/core/deepfist/DeepFistStream.{h,cpp} |
spanConfidence(), optional pieces out-param |
Yes | In scope |
src/core/deepfist/DeepFistCommitter.h |
Sighting history, per-letter mean, Piece |
Yes | In scope; Blocker 1 |
src/models/DeepFistCwModel.{h,cpp} (signal, per-piece emit) |
scoredTextDecoded |
Yes | In scope |
src/models/CwRxModel.{h,cpp} |
coloredTextDecoded, generation-guarded like its siblings |
Yes | In scope |
src/gui/MainWindow.h, MainWindow_DigitalModes.cpp |
Slot + UniqueConnection |
Yes | In scope |
src/gui/PanadapterApplet.{h,cpp} |
cwCostColor() extraction, appendColoredCwText() |
Yes | In scope; Blocker 2 |
tests/deepfist_committer_test.cpp |
Confidence arithmetic cases | Yes | In scope |
DeepFistCwModel::appParameters(), tests/deepfist_cw_model_test.cpp, tests/tests.cmake, half of docs/deepfist-cw-backend.md |
#5953's threshold change | Disclosed stack | Belongs to #5953 |
- New surface: three internal C++ signals and two static helpers. There is no settings key, wire verb or capability.
- Not in the diff: no CHANGELOG entry.
- Consumers:
unscoredTextDecodednow has no producer, which the body discloses. I checked for any other consumer of DeepFist text (MQTT, spotting, logging). The panel is the only one onmainand here, so rerouting drops nothing. - Step-0 preflight:
deepfist_committer_testlinksQt6::Coreonly; no socket or fake peer.
Blockers
1. src/core/deepfist/DeepFistCommitter.h:30-38: a repeated letter takes its neighbour's score
The sighting scan walks m_history newest-first and takes the first same-ID sighting within 0.12 s from each earlier window. A window's tokens are pushed in time order, so the reverse walk meets the following letter first. Two Es 80 ms apart, at 0.9 and 0.1, seen identically in two windows. Compiled against this head's header:
'E' 0.500 ← should be 0.900 (both of its sightings were 0.9)
'E' 0.100
So a confident letter shows orange because it averaged in its neighbour.
- When it happens: adjacent same letters 80–120 ms apart are fast machine CW (an
EEpair is about 120 ms apart at 40 WPM). - Jitter makes it reachable at ordinary speeds: the comparison is across windows, so any jitter in a letter's time estimate between windows shrinks the gap.
- What it costs: this is the exact property #6099 asks for.
- The test misses it: the existing test spaces its equal-ID cases far apart and passes on this head.
Drop-in replacement for the std::vector<double> ticks; for (…rbegin…) {…} block. It picks the closest sighting per earlier window:
// Per earlier window, the closest sighting of the same token: a repeated
// letter inside the tolerance must not take its neighbour's score.
std::vector<std::pair<double, const Sighting*>> closest;
for (const Sighting& sighting : m_history) {
const double distance = std::abs(sighting.seconds - token.seconds);
if (sighting.id != token.id || distance > 0.12) { continue; }
const auto window = std::find_if(closest.begin(), closest.end(),
[&](const auto& entry) { return entry.second->end == sighting.end; });
if (window == closest.end()) { closest.push_back({distance, &sighting}); }
else if (distance < window->first) { *window = {distance, &sighting}; }
}
for (const auto& entry : closest) {
sum += entry.second->confidence;
++count;
}And a regression case for tests/deepfist_committer_test.cpp, just before the final std::puts:
// A repeated letter inside the 0.12 s tolerance keeps its own score.
DeepFistCommitter closePair;
pieces.clear();
closePair.process({{6, 2.80, "E", 0.9f}, {6, 2.88, "E", 0.1f}}, false, 4.0, 2.0, false, &pieces);
closePair.process({{6, 2.80, "E", 0.9f}, {6, 2.88, "E", 0.1f}}, false, 4.4, 3.0, false, &pieces);
if (pieces.size() != 2 || !near(pieces[0].confidence, 0.9f) || !near(pieces[1].confidence, 0.1f)) {
return 24;
}I ran both:
- This head plus the case: exit 24.
- With the fix: exit 0, the probe prints
0.900/0.100, and every existing case passes. - With the fix and the sighting history removed: still exit 22, so your mutation check keeps working.
2. src/gui/PanadapterApplet.cpp:931: DeepFist text drops below the contrast canon in Light themes
The old path drew DeepFist text with color.text.primary (#1a2a3a, 13.44 : 1 on Default Light's #f5f5f8). The new path uses the ggmorse literals. Computed with the WCAG formula against that background:
| band | hex | Default Light |
|---|---|---|
| green | #00ff88 |
1.23 : 1 |
| yellow | #e0e040 |
1.29 : 1 |
| orange | #ff9020 |
2.08 : 1 |
| red | #ff4040 |
3.18 : 1 |
By this PR's own tables about 85 % of letters are green, so most DeepFist output becomes close to invisible in Light. The previous ten9876 review captured the panel on the demo and saw exactly that.
docs/a11y.md:143 sets "4.5 : 1 for normal text". docs/a11y.md:177 allows concrete hex only as "Concrete hex that meets the ratio, file an issue to add the token". None of the four meets it in Light, so the intermediate-state allowance does not apply.
- Why ggmorse doesn't excuse it: ggmorse already has the same problem, but that is a separate defect. This PR moves DeepFist from a compliant token onto it.
- Fix: resolve the four bands through theme tokens with Light values that meet 4.5 : 1, and verify both themes. Or, if you want to keep this PR narrow, keep
color.text.primaryfor DeepFist in Light until the tokens exist. - Also @ten9876: colour assignment is RFC-scoped visual design (
GOVERNANCE.md), which the body correctly flags.
Nits (non-blocking)
DeepFistCommitter.h:33(0.12 s) vs the pending matcher'sdistance = 0.16at:66. A letter whose time estimate drifts 0.12–0.16 s between windows is still carried as the same pending character, but loses its earlier sightings and shows its single-window score. Share one constant, or say in the comment why the confidence window is tighter.DeepFistStream.cpp:49-66:spanConfidence()has no test. It is pure and static, butDeepFistStream.cppcompiles only underENABLE_DEEPFIST_EXPERIMENT, which no workflow sets. Making it header-only would put it ondeepfist_committer_test's unconditionally-registered lane.Token::confidence = 1.fdefault. A producer that forgets to set it gets green.0.fwould fail safe; onlyDeepFistStreamand the tests construct Tokens today.- Body: "same scorer used in #5950/#5953". As the previous review noted, the bundle's
score_pieces.pynormalises per piece withouthyp=True, so the CER definitions differ. Please qualify the sentence.
What I verified, what held, what I could not test
- Ran
deepfist_committer_testat this head, compiled standalone from the head's sources with Qt 6 Core, the same link line as itstests.cmakeblock. It passes.- Your mutation: removing the sighting history gives exit 22, as the body claims.
- Blocker 1: reproduced with a probe against the head's header. The fix and the regression case are validated as stated above.
- Contrast figures recomputed from the hex values; they match the previous reviews.
- Merge state.
- vs
main: the head's merge base is 52 commits behindmain(85f2a124).git merge-treeis clean, andmainhas touched none of the DeepFist decoder files since. - CI: earliest run on this head was created 2026-10-02 22:46Z, before
main's tip. No workflow setsENABLE_DEEPFIST_EXPERIMENT, so green only proves the default build compiles the unconditional signal and thecwCostColor()refactor.
- vs
- Held.
- Every
publish()/separate()writes bothoutputandpieces, so text and colour cannot diverge. spanConfidence()cannot divide by zero, and returns0.f(red) for out-of-range input.- NaN costs fall through every band to red.
- The new connection uses the same generation/
isRunning()guard as its siblings. m_historyis pruned at 8 s and dies with the stream on every generation change.
- Every
- Not run. There is no DeepFist model on this host (
DEEPFIST_MODEL_BASE_URLis empty by design), so I did not run inference or drive the DeepFist panel. The Light-theme screenshot evidence is the previous ten9876 review's. I did not reproduce the replay tables, so the per-colour accuracy figures are the author's.
Recommendation
Request changes. Take the closest-match fix and its test, and keep DeepFist text at or above 4.5 : 1 in Light. Then this waits on two maintainer calls: #5953's threshold, which has to land or be refused first, and the colour-assignment decision under GOVERNANCE.md.
— Jeremy KK7GWY + Claude
…noise-only arm (aethersdr#5950). Principle VIII. A failed decode in the threshold-12 control arm read as "the gate closed" and passed. The same constructed noise with the keying removed must not print more with the app's parameters than at 12. The carrier arm's comment names the completed-mark guard, which holds a carrier at any threshold. The Parameters comment no longer says the app uses the defaults unchanged. docs: the threshold is the value every user runs, not a developer setting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/deepfist-cw-backend.md
…dr#6099). Principle VIII. Same letters chained within 0.12 s pair with an earlier window's sightings by order when the counts match, else each takes the closest sighting. The newest- first scan gave a fast EE's first letter the second's score; closest alone swaps again once the windows' times shift by half the spacing (test 25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Light (aethersdr#6099). Principle VIII. Four tokens, color.cw.confidence.{high,medium,fair,low}: Dark keeps the four ggmorse colours; Light uses its own green and red and two darker values for yellow and orange, each at least 4.5 : 1 on color.background.0. ggmorse keeps its literals. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@ten9876 Both blockers addressed in — authored by agent (Claude Code) on behalf of @skerker |
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
@skerker — #5953 landed today (squash abc5b26e7, 2026-10-06 20:32Z), so this PR is unblocked and ready for your final work. I reviewed e1eb9a9e independently and found no new blocker. I'll come back to approve and merge once it is in a mergeable state.
Both earlier blockers are fixed
- Repeated letters. The new test exits 24 against the old matcher and 0 on
e1eb9a9e. Your mutations reproduce on my side (closest-only → 25, history ignored → 22). On a synthetic stream rich in same-letter pairs, letters landing in the wrong colour band dropped from 13.7 % to 5.2 %. - Light-theme contrast. I recomputed the ratios from the tokens as the real
ThemeManagerresolves them (widget-scoped lookup included): Light is 4.59 / 4.61 / 4.64 / 5.56 : 1 on#f5f5f8, all ≥ 4.5. Dark is unchanged. The theme-seed, test-registration and CI-gate checks pass, andtheme_seed_testandtheme_manager_testpass from this head. - The default build compiles the touched files, and the
HAVE_DEEPFISTpaths pass-fsyntax-only.
What's needed to get it mergeable
- Bring in
main. The PR isCONFLICTINGin three files:resources/themes/default-dark.json,default-light.json(#6198 added tokens in the same place as yours) anddocs/deepfist-cw-backend.md(#5953 landed as a squash, and your rewritten next sentence collides with it). Keep both sides' tokens and keep your rewrite of "displayed without inventing confidence". The #5953 commits then drop out of the diff. Merge or rebase is fine; the repo squash-merges. - Let CI run, and refresh the body. No check has run on this head, because a conflicting PR has no merge commit to test. The body still says the branch is stacked on an open #5953 and that CI has not run; both are now out of date.
- Push last, then ping us. Branch protection dismisses approvals on push, so we'll approve against your final head.
- @ten9876's two
CHANGES_REQUESTEDreviews still stand. They were written against4c9a73e5; the colour assignment is also the visual-design call underGOVERNANCE.md:97. Our approval won't clear his reviews, so a re-review from him will be needed.
Optional, non-blocking
- Matcher fallback. When the earlier window saw a different number of the same letter, each letter takes the closest sighting and one sighting can serve two letters. Case 26 pins that: the second
E(0.1) shows 0.5 by borrowing its neighbour's 0.9. Keeping your order rule and making the fallback one-to-one (each sighting used at most once) lowered the band-mismatch rate from 5.2 / 5.6 / 10.1 % to 2.3 / 3.2 / 8.5 % at ±30 / ±50 / ±80 ms jitter. That is synthetic data, and your 9,105 real letters showed no change between the two heads, so it is rare in practice. - Duplicated band thresholds. 0.15 / 0.35 / 0.60 now live in both
cwCostColor()andappendColoredCwText(); one shared band helper would keep them from drifting. - Still open from earlier rounds:
Token::confidencedefaults to1.f, which shows green if a producer forgets to set it;0.ffails safe.spanConfidence()has no direct test.- The sighting window is 0.12 s while the pending matcher uses 0.16 s.
- History in a comment. The "the review's case" comment in
tests/deepfist_committer_test.cppis review history, which AGENTS.md keeps out of code.
What I did not verify
I couldn't run DeepFist inference here (no model or ONNX Runtime on this machine), so the replay table, the live FLEX smoke test and the panel grabs are yours, not reproduced.
Thanks for the quick turnaround on the review round.
—
73,
Ryan NF0T
👨🏼💻 Co-authored by Claude Sonnet 5.5
# Conflicts: # docs/deepfist-cw-backend.md # resources/themes/default-dark.json # resources/themes/default-light.json
|
@NF0T @ten9876 — ready at — authored by agent (Claude Code) on behalf of @skerker |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed ee5c816b992c589538caf0afc9afb683497123f4.
Approve. This is the change #6099 asks for: each DeepFist letter is coloured from the model's own posterior, on the four bands the CW panel already uses, with the sensitivity slider still not hiding DeepFist text. ggmorse still goes through appendCwText() and the same four literals. A default build stays off the experiment; CI on this SHA is green (build, check-macos, check-windows, Static checks).
Scope
| Group | Files | Fit |
|---|---|---|
| Score | DeepFistStream::{h,cpp} spanConfidence(), DeepFistCommitter.h sighting mean |
#6099's mean posterior, then the mean across windows |
| Delivery | DeepFistCwModel, CwRxModel, MainWindow_DigitalModes.cpp, PanadapterApplet |
Separate coloredTextDecoded path; Sens filter and cwRxTextDisplayed stay on the ggmorse path |
| Theme | default-{dark,light}.json, ThemeSeedGenerated.cpp, canonical-tokens.md, docs/deepfist-cw-backend.md |
Four color.cw.confidence.* tokens; doc matches the new behaviour |
| Test | tests/deepfist_committer_test.cpp |
Existing registered target, extended. No socket |
merge-tree against main (43e2633e) is clean. This head is 10 commits behind; the DeepFist files do not collide.
Earlier review items
Both changes requested against 4c9a73e5 are on this head. Repeated letters 80 ms apart keep their own scores (test exits 24 on the old matcher). Light-theme tokens on color.background.0 #f5f5f8 measure 4.59 / 4.61 / 4.64 / 5.56 : 1 (#1a8040, #737313, #ae5700, #c02020). Dark is the existing ggmorse literals #00ff88 / #e0e040 / #ff9020 / #ff4040 (14.19 / 13.51 / 8.39 / 5.49 : 1 on #0f0f1a).
Nits (non-blocking)
tests/deepfist_committer_test.cpp:112says "the review's case". That is review history; the numbers in the assertion are the durable part.Token::confidencedefaults to1.f(DeepFistCommitter.h:14), so a forgotten score is green.0.fwould fall through to red.- The 0.15 / 0.35 / 0.60 cuts live in both
cwCostColor()andappendColoredCwText(). - Sighting identity is 0.12 s; the pending-character match is 0.16 s. A letter can stay the same pending character and still lose earlier sightings. The header comment states the 0.12 s rule.
Verification
deepfist_committer_test at this SHA, Qt 6.12, QT_QPA_PLATFORM=offscreen: pass. Forcing closest-sighting for every window (no order match) exits 25. Restored sources pass again. The test binary has no socket.
Not run: DeepFist inference, the held-out W1AW colour table, and the panel itself. Those figures stay the author's. spanConfidence() is compiled only with ENABLE_DEEPFIST_EXPERIMENT, which no workflow sets, so this run does not execute it. Reading it: the row is max-subtracted, the starting frame is always counted, and an empty or out-of-range span returns 0.f.
Follow-up (not a merge condition)
spanConfidence() still has no direct test. A 3×3 float fixture on the committer's always-built target would pin the span walk, the softmax, and the 0.f reject. That needs the function visible without the experiment (header-only, or a small extracted helper). No firmware peer.
4 participants
Fixes #6099. Builds on #5953 (merged 2026-10-06 as
abc5b26e). This PR's commits:41a889ef,4c9a73e5, and the review round34bea416(Blocker 1) ande1eb9a9e(Blocker 2).ee5c816bmergesmain(25de97c3): the two theme files keepmain's new token blocks beside this PR'scwblock, and #5953's change has dropped out of the diff.Summary
Each DeepFist letter in the CW decode panel is now coloured by the model's own confidence, in the four colour bands the panel already uses for ggmorse, drawn from four theme tokens: Dark keeps the ggmorse colours, Light gets readable values. The score is the mean posterior over the letter's frames, averaged over the overlapping decode windows that saw it before it settled; cost = 1 − score; the existing bands (< 0.15 green, < 0.35 yellow, < 0.60 orange, else red) apply. Nothing is hidden: the sensitivity slider stays disabled for DeepFist, as before. ggmorse display, callsign spotting and MQTT output are unchanged. DeepFist remains off by default (
ENABLE_DEEPFIST_EXPERIMENT), so a default build is unchanged.src/core/deepfist/DeepFistStream.{h,cpp}(spanConfidence()),DeepFistCommitter.h(Piece, sighting average),src/models/DeepFistCwModel.{h,cpp}(scoredTextDecoded),src/models/CwRxModel.{h,cpp}(coloredTextDecoded),src/gui/PanadapterApplet.{h,cpp}(cwCostColor(),appendColoredCwText()),src/gui/MainWindow_DigitalModes.cpp,resources/themes/default-{dark,light}.json(color.cw.confidence.*)HAVE_DEEPFIST/ the DeepFist sources; a default build also gets the unconnectedCwRxModel::coloredTextDecodedsignal (besideunscoredTextDecoded) andcwCostColor(), the ggmorse band table moved into a helper with no behaviour changetests/deepfist_committer_test.cpp(existing registered target, extended)Kept on purpose: the unscored path (
CwRxModel::unscoredTextDecoded,PanadapterApplet::appendUnscoredCwText) has no producer after this change — DeepFist was its only one — and stays as #5716's contract for a backend that reports no per-letter score; its comment now says so.Left out: no sensitivity filtering for DeepFist (hiding helps mainly on weak signals; marking does not lose text); no claim for hand-sent code; no change to DeepFist's release status (#4817).
Design choices
DeepFistStream::spanConfidence()), then the mean over its sightings in successive windows (DeepFistCommitter, same token within 0.12 s). Same letters chained within 0.12 s in one window pair with an earlier window's sightings by order when the counts match, otherwise each takes the closest sighting; closest alone gives a fastEEswapped scores once the windows' times shift by half the spacing (test case 25). Chosen by measurement on two W1AW takes (the sighting mean separated correct from wrong copy better than a single window's value).cost = 1 − scoreso the existing panel contract (lower is better,PanadapterApplet.cppband table) applies unchanged — the convention RFC: DeepCW neural CW decoder as an optional second CW backend #4817's review set for a neural backend.coloredTextDecoded(text, cost)besidetextDecoded/unscoredTextDecodedonCwRxModel, because this cost is on the backend's own scale and must never meet the Sens threshold thatappendCwText()applies to ggmorse's cost.color.cw.confidence.{high,medium,fair,low}, added to both themes (docs/style/theme-style-guide.md§4), seed regenerated, recorded indocs/theming/canonical-tokens.md. Dark = the ggmorse colours (#00ff88 / #e0e040 / #ff9020 / #ff4040). Light =#1a8040(the theme's green) /#737313/#ae5700/#c02020(the theme's red), 4.59 / 4.61 / 4.64 / 5.56 : 1 oncolor.background.0#f5f5f8, the CW panel's background. In Light the yellow and orange are darker than their Dark counterparts (≥ 4.5 : 1 needs that); green and red read clearly. ggmorse keeps its literal table (hardcoded-colour ratchet +0); moving it to the same tokens is a separate follow-up.Color, matching the codebase.docs/deepfist-cw-backend.mdnow says letters are coloured by the model's posterior and never filtered by the sensitivity slider (it previously said output is displayed "without inventing confidence").CHANGELOG.mduntouched.Governance note: colours are on the RFC list; this change reuses the existing bands on an off-by-default experiment and adds four theme tokens (Dark = the existing ggmorse colours, two new Light values) and no dependency, thread, setting or default. DeepFist itself lives under RFC #4817, where this is the decode-quality work #6099 cites. Stated here for the maintainers' call.
Relation to the triage on #6099
The triage's sketch and this branch agree on: the score computed in
DeepFistStream.cpp, not the vendored files; a separate per-piece signal carrying (text, cost) pairs rather than a widenedtextDecoded; a separate DeepFist append with no Sens filter and nocwRxTextDisplayed; the band table factored into a helper. Two differences: (1) the cross-window average does not go throughm_pending— the committer keeps a short sighting history (same token id within 0.12 s, pruned after 8 s) that the commit-tick path reads, so the letter that commits on a tick does get its earlier windows; the extended test pins that case. (2)spanConfidence()normalises each frame row (max-subtracted softmax), so it yields the same value whether the model export hands back log-probabilities or raw logits; no row-sum test is needed for that.Constitution principle honored
Principle VIII — the colour is a measured quantity from the model's own output, scored against published bulletin text before and after the change, with the evidence attached.
Test
deepfist_committer_test(socket-free, Qt Core only, registered intests/tests.cmake, builds with the experiment off): a published letter carries the mean over its sightings; the separator is shown as certain; a carried letter averages the sightings it had. Fixtures are CONSTRUCTED values exercising the arithmetic only.E80 ms apart at 0.9 / 0.1 keep their own scores (case 24, the review's case verbatim); the same pair with the later window shifted 50 ms (case 25); an earlier window that saw one of the two takes the closest-sighting fallback (case 26).e1eb9a9e(logs/mutation-check.login the review-round bundle): head4c9a73e5's matching restored → exit 24; closest-only → 25; sighting history ignored → 22; restored → 0.e1eb9a9e: 609 tests, 606 pass, 2 pre-existing skips, 1 fail —hl2_receiver_count_restart_paced_test, a timing check that ran while eight replays loaded the machine; it passes alone (logs/ctest-full.log,logs/hl2-paced-rerun.log). Repeated atee5c816bafter themainmerge: 668 tests, 665 pass, 2 pre-existing skips, 1 fail —pgxl_panel_test(:553/:554, the PGXL panel layout check frommain's fix(pgxl): fit the docked panel's controls and readings. Principle XI. #6036); this PR touches no PGXL file.full-suite.ymlat merge and weekly onsanitizers.yml; it is not on the frozen per-PR gate (ci: freeze the per-PR test gate; the full suite runs weekly on sanitizers.yml. Principle VIII. #5405).ENABLE_DEEPFIST_EXPERIMENTis not set in any workflow, so hosted builds compile the experiment off. CI atee5c816b: all five checks pass.main), test registration, touchpoint manifest, CI test gate, capability records, command plane — all pass.Proof
Replay evidence from W1AW over-the-air recordings: real band conditions (noise, fading, nearby signals) captured earlier on a FLEX-8400, receive only, and replayed offline. A local replay tool compiled against this head's
DeepFistStream/DeepFistCommitterfeeds each recording through the same path the app's worker uses and writes every letter with the confidence the panel colours it by; letters are scored right/wrong against the ARRL bulletin text with the scoring helpers used in #5950/#5953. Percent of decoded letters correct per colour (letters in brackets). The table reproduces exactly ate1eb9a9efrom the attached audio (the four bulletins, cut as the README states);4c9a73e5ande1eb9a9egive identical text and 0 confidence changes in 9,105 letters:The ARLD038 weak row differs from the figures in #6099 (96.7 / 75.6 / 54.3 / 32.6 %, CER 49.8 %): those came from the Phase 0 trace on 2026-09-26; this row is re-scored at this head from the shipped committer's own output.
Three minutes of the held-out ARLP038 take at three signal levels (bar = CW tone over noise; grey = the bulletin text; underlined = wrong):
At 25.7 / 20.0 / 16.7 dB: 83 / 78 / 32 letters decoded, 1 / 7 / 5 wrong, 2 % / 21 % / 46 % not green. A letter the decoder never produced has no colour; on the weak minute the missing words show only as gaps against the bulletin line.
Live smoke at
e1eb9a9e(About grab in the bundle): FLEX-8400, live 40 m CW, receive only, DeepFist selected. Pixel colours read from the CW panel grabs: Dark =#00ff88 / #e0e040 / #ff9020 / #ff4040on#0f0f1a; Light =#1a8040 / #737313 / #ae5700 / #c02020on#f5f5f8; all four bands appear in each. The bundle'spages/draw the four recordings in both themes.Evidence bundle (pages, per-letter CSVs, tools, truth texts, logs):
aethersdr-issue6099-deepfist-letter-colours-2026-10-02.zip— head4c9a73e5· four replay pages + CSVs, scorer, mutation and suite logs, demo grabs.aethersdr-pr6103-review-round-2026-10-06.zip— heade1eb9a9e· old/new-head replays + comparison, scoring, Dark/Light pages, mutation and suite logs, live-smoke grabs and app log, audio manifest.…-audio-…wav.xz.part0/1.zipfiles = the four bulletins analysed (two parts each; reassembly and sha256 in the review-round README).aethersdr-issue6099-deepfist-letter-colours-2026-10-02.zip
aethersdr-pr6103-review-round-2026-10-06.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arlp038-2026-09-24-15m-bulletin-mono-24k.wav.xz.part1.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arlp038-2026-09-24-15m-bulletin-mono-24k.wav.xz.part0.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arlp039-2026-09-26-40m-bulletin-mono-24k.wav.xz.part1.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arlp039-2026-09-26-40m-bulletin-mono-24k.wav.xz.part0.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arld038-2026-09-25-17m-weak-bulletin-mono-24k.wav.xz.part1.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arld038-2026-09-25-17m-weak-bulletin-mono-24k.wav.xz.part0.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arld038-2026-09-24-17m-bulletin-mono-24k.wav.xz.part1.zip
aethersdr-pr6103-review-round-2026-10-06-audio-arld038-2026-09-24-17m-bulletin-mono-24k.wav.xz.part0.zip
Other backends / other platforms
appendCwText), the band table only moved. TX-side decode (appendCwTextTx) unchanged.What I tried to break
CwRxModel::bind()is the same one the other two signals use); Windows and macOS builds.Test plan
ee5c816b, Linux,-DENABLE_DEEPFIST_EXPERIMENT=ONee5c816b(Test); CI passes atee5c816bChecklist
docs/COMMIT-SIGNING.md)AppSettingscalls — use nested-JSON-under-one-key (Principle V) — no settings touchedMeterSmoother(AGENTS.md convention) — n/a, no meterdocs/deepfist-cw-backend.md;CHANGELOG.mduntouched— authored by agent (Claude Code) on behalf of @skerker