Repository navigation
Say what shift the sessions could have shown, and warn when it is too coarse - #24
Conversation
… coarse A verdict of nothing found means nothing at all unless a shift worth finding could have been seen. Two sessions of a few seconds will report no significant shift whatever the generator did, and that reading was being left to stand on its own as though it said something about the generator rather than about the length of the recording. SmallestDetectableDifference is the half width of the confidence interval for the pair, which is the test read the other way round: rather than asking whether the shift that happened beats the noise, it asks how large a shift would have to be before it could. A test pins the two together by shifting readings by exactly that much and checking the probability lands on the significance level. The verdict states it either way. Below SHIFT_OF_INTEREST, one part in ten thousand and the order of the effect reported in the published work, it says so in the warning colour and asks for a longer recording. The three outcomes are three weights, which is what VerdictWeights carries. A shift found is notable however short the sessions, because it cleared the limit by being found at all. Nothing found from sessions that could have found something is the ordinary outcome of the experiment. Nothing found from sessions that could not is the one that misleads, and is the only one that warns. Worked out from each session's own spread rather than from a count of readings, because a device reading averages 262,144 bits and a simulated one 16,384: the simulated spread is four times wider and needs sixteen times the readings to pin its mean down as finely. A fixed count would have meant different things for the two. The verdict band had to grow with the wording. It was two lines and the warning needs three, so it was clipped mid-sentence when first run - which only showed up in the built application, as the tests read the text rather than look at it. Sizing the label to its content was worse: an AutoSize label grows sideways rather than wrapping, so it left the card entirely. It is a taller fixed band, checked against the narrowest the window is allowed to be, where the wording needs 45px of the 78 it now has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are cohesive, well-tested (unit + manual), and the only noted issue is a minor misleading comment in the constants block.
Pull request overview
This PR improves the analysis verdict UX by explicitly stating how small a baseline/result shift the sessions were capable of detecting, and warning when “no significant shift” is not informative due to insufficient sensitivity.
Changes:
- Add sensitivity math to the stats layer (
SmallestDetectableShift,SmallestDetectableDifference,SHIFT_OF_INTEREST) and expose a simple “sensitive enough” predicate. - Update the Analyze tab verdict wording/weighting and adjust layout so multi-line warnings are visible.
- Add coverage via new component tests and a new manual suite (
check-sensitivity.ps1) that drives the UI through the three verdict outcomes.
File summaries
| File | Description |
|---|---|
| tests/RandomNumberGenerator.Test/SignificanceTest.Test.cs | Adds new component tests validating detectable-shift math and sensitivity threshold behavior. |
| tests/manual/run-all.ps1 | Registers the new sensitivity manual suite in the aggregate runner. |
| tests/manual/README.md | Documents the new manual suite and what it verifies. |
| tests/manual/check-sensitivity.ps1 | New manual harness that validates verdict wording and color across key scenarios. |
| SignificanceTest.cs | Implements detectable-shift/difference calculations and defines SHIFT_OF_INTEREST. |
| README.md | Explains the new “enough readings to answer” concept in the project overview. |
| GeneratorForm.Designer.cs | Increases Analyze-tab verdict area to prevent clipping. |
| GeneratorForm.cs | Updates verdict construction, adds VerdictWeights, and applies color/font emphasis consistently. |
| CLAUDE.md | Updates architecture documentation to describe the new sensitivity-based verdict behavior. |
Review details
Files not reviewed (1)
- GeneratorForm.Designer.cs: Generated file
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The comment about needing two readings to measure a spread belongs to the minimum readings constant below it, not to the shift being looked for. It was left stranded there by the edit that added the new constant: it anchored on the declaration alone, so the comment that was already above it stayed where it was and a fresh copy went in with the declaration that owned it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
There was a problem hiding this comment.
🟡 Changes recommended
The updated verdict path introduces additional full-pass computations over session readings that should be refactored to avoid unnecessary repeated O(n) traversals for larger sessions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- GeneratorForm.Designer.cs: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/RandomNumberGenerator.Test/SignificanceTest.Test.cs:647
- The
MakeSpreadReadingsdoc comment says the spread is "exactly" what was asked for and documentsfSpreadas a standard deviation, but the implementation actually usesfSpreadas the fixed offset amplitude above/below the mean (the resulting sample standard deviation will be close, but not exactly equal). Adjusting the wording avoids misleading future test readers.
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Lite
The verdict was asking for the detectable difference separately from the test it sits beside, which walked both sets of readings a second time to work out quantities the test had already worked out: the means, the variances, the combined error and Welch's degrees of freedom. Measured, the second walk costs under four milliseconds for an eight hour session, on a path that runs when a file is loaded rather than while anything is recording, so the cost was not the reason to change it. The reason is that the two were separate copies of the same arithmetic and had to agree. The test that shifts readings by exactly the detectable amount and expects the probability to land on the significance level only passed because they did; change Welch's degrees of freedom in one and the verdict would have quoted a limit that disagreed with the test printed beside it. The limit now comes back with the result, worked out where the probability is worked out and from the same critical value, so they cannot drift apart. The standalone calculations are gone and the tests go through the tests. An outcome with too little to test reports no answer for the limit rather than zero, which is why the invalid returns go through one place now. Zero would have read as "a difference of any size would have been found", and anything asking whether the readings were sensitive enough would have agreed with it. Also corrects the test helper's description: what it takes is how far each reading sits from the mean, not the standard deviation the readings end up with. Those are close but not equal, the sample deviation being the larger for dividing by one fewer than the count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete documentation/test-structure convention issues in the changed files (e.g., stale identifier in CLAUDE.md and a non-conforming test name/placement) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- GeneratorForm.Designer.cs: Generated file
- Files reviewed: 8/9 changed files
- Comments generated: 3
- Review effort level: Lite
Three points from the review, all of them right. CLAUDE.md still named SmallestDetectableDifference, which the last change removed. It reads SignificanceResult.DetectableDifference now, and the paragraph around it says where the value is worked out. The tests added for the detectable difference were sitting inside the constants region rather than the tests region. The splice that added them anchored on the last endregion in the file, which closes the constants rather than the tests, so seven tests and a helper landed under a label that says none of them are there. They are now at the end of the tests region, after the other helper, which is where this file keeps them. The same splice put two tests in DeviceUpdateThread.Test.cs inside its helper types region. That file is not part of this change and the review could not see it, but it is the same mistake from the same edit and is a move of two methods, so it is corrected here rather than left to be found later. Tests_NullReadings_Exception was named after nothing: there is no method called Tests. It covered three cases, one of which already had a test of its own. It is two tests now, named for the method and the case each one covers, and the duplicate is gone. 252 tests pass on both configurations. No application code changed, so the verdict path is as it was; the sensitivity suite was run against the built application to confirm it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent and well-covered by added automated and manual checks, with only a minor style-level convention issue noted.
Review details
Files not reviewed (1)
- GeneratorForm.Designer.cs: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
GeneratorForm.cs:2493
- Repository coding conventions call for avoiding method calls inside conditionals (CLAUDE.md:281).
test.Significantis a computed property (getter executes logic), so consider assigning it to a localboolfirst and using that in theiffor consistency with the rest of the codebase’s style rules.
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Round 4: 🟢 approval recommended, with one suppressed comment about The rule it cites is titled Function Calls in Conditionals and reads "Do not make function calls inside conditional statements". Its worked example is a method call, and its stated reason is breakpoint placement: // Incorrect - function call inside conditional
if (m_Reader.ReadToFollowing("Session"))
// Correct - extract function call
bool bSessionFound = m_Reader.ReadToFollowing("Session");A property read is not what that rule is about, and the codebase is consistent on the point:
Two lines above the flagged one, my own code has So extracting this one property read to a local would make the line less consistent with the file around it, not more. It would single out one property among many for treatment the others do not get. If the intent is that computed properties should be extracted too, that is a change to the guideline and to roughly a dozen existing sites, not to this line alone — worth doing deliberately if wanted, but not as a side effect of this pull request. Nothing else was raised. Stopping the review loop here. |
A verdict of nothing found means nothing at all unless a shift worth finding could have been seen. Two sessions of a few seconds will report no significant shift whatever the generator did — and that reading was being left to stand on its own, as though it said something about the generator rather than about the length of the recording.
What it does
The verdict now always states the smallest difference the two sessions could show as significant, and warns when that is coarser than the shift being looked for.
Too short — the case this exists for, in the warning colour:
Long enough, nothing found — the ordinary outcome:
A shift found — the limit is reported, not warned about:
How the limit is worked out
SmallestDetectableDifferenceis the half width of the confidence interval for the pair — the test read the other way round. Rather than asking whether the shift that happened beats the noise, it asks how large a shift would have to be before it could. A test pins the two together: shift the readings by exactly that much and the probability lands on the significance level.The bar is
SHIFT_OF_INTEREST, one part in ten thousand — a generator running at 0.5001 rather than 0.5, the order of the effect reported in the published work. About 40 seconds of device recording clears it.It is derived from each session's own spread, not from a count of readings, because the two sources differ: a device reading averages 262,144 bits and a simulated one 16,384. The simulated spread is four times wider and needs sixteen times the readings to pin its mean down as finely, so a fixed reading count would have meant quite different things for the two.
Three outcomes, three weights
VerdictWeightscarries them. Worth being precise about which one warns:The layout fault this turned up
The verdict band was two lines and the warning needs three, so it was clipped mid-sentence the first time it ran. That only showed up in the built application — the suites read the verdict text, and the text was correct; it was the band that was too short.
Sizing the label to its content made it worse: an
AutoSizelabel grows sideways rather than wrapping, so it left the card entirely and only a fragment showed at the edge. It is a taller fixed band instead, checked against the narrowest the window is allowed to be, where the wording needs 45px of the 78 it now has.Verification
check-sensitivity.ps1is new: it drives the analysis tab through all three verdicts and checks the wording, the numbers and the colour of each.🤖 Generated with Claude Code
https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w