feat: lower the confidence floor to 0.15, and make the re-sweep gate notice #196
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/lower-confidence-floor"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's in
The owner paged through the library with the overlay on — the deliverable phase
1 existed to produce — and reported the shape of the error: few false
positives, many missing detections. Measured against that, and two things came
out of it.
The floor was leaving text behind
120 random pages of the real library (seed 23, the same draw as ADR 0027's
tables), one inference pass each at a 0.02 floor so every candidate threshold is
counted from the same detections:
The useful column is the last one. Lowering the floor almost never finds a
new page — 43 of 120 carry text at 0.25 and still 43 at 0.07. What it finds is
more text on pages already known to have some: sound effects, margin notes,
narration over art. Exactly the class a balloon outline does not contain, and
the one phase 3 needs most.
What the old floor was dropping, by band:
0.15 rather than 0.10 is the owner's call — the conservative step, taken with
the numbers in view. Below 0.05 a long tail appears (151 further text
detections between 0.02 and 0.05) that looks like noise.
The change would have done nothing
This is the part worth reading. The detector version was the model file's
fingerprint alone:
So lowering the floor and restarting would have changed nothing: every image
still carried an analysis at the unchanged version, the sweep correctly
concluded it had finished, and the new floor never ran. No error, no log line,
no regions. The same was true of
DOUJIN_MANAGER_DETECTION_IMAGE_SIZE.The regions on disk are a function of the model and the settings that shape
what comes out of it, so the version now says so:
Composite and readable rather than a second hash over the three, so a row in
image_analysesand a line in the log say why a re-sweep happened. Formatted:R— round-trippable — because a rounded format puts the same defect back oneedge out:
0.123456and0.123457both render as"0.1235", and two differentfloors sharing a version is exactly the silence this fixes.
DOUJIN_MANAGER_DETECTION_THREADSis deliberately excluded. It decides howfast a page is examined, never what is found on it, and folding it in would
re-sweep twelve thousand images because an operator gave the sweep another core.
Verification
scripts/verify.shgreen (== verify OK (all) ==) — backend 1,526 passed / 5skipped, app 1,562, analyze clean.
New directional tests, all reachable without a model on disk — which matters,
see below:
A_Different_Confidence_Floor_Is_A_Different_VersionA_Different_Input_Size_Is_A_Different_VersionTwo_Floors_That_Agree_To_Four_Places_Are_Still_Two_VersionsA_Floor_Near_One_Is_Not_Rendered_As_One0.99996rendering as a value the config refusesA_Different_Thread_Count_Is_The_Same_VersionA_Different_Model_Is_A_Different_VersionA_Built_Detector_Reports_The_Model_And_The_Settings_As_Its_VersionThe_Settings_That_Shape_A_Detection_Default_To_What_Is_DocumentedThe_Version_Says_Which_Settings_Produced_It/..._Fits_The_Column_It_Is_Stored_InHasMaxLength(100)Mutation-probed: 9 mutations, all killed — re-run with
DOUJIN_MANAGER_DETECTOR_MODELunset, the way CI runs.Consequence: the first start re-sweeps
About an hour for 12,468 pages at the measured 309 ms each. That is the
mechanism ADR 0027 designed for a model swap, doing its job for the first time.
The Detection section shows it working through the library, and the old regions
are replaced per image as it goes — nothing is deleted up front.
Recorded as ADR 0027 Amendment 2, with the measurement tables and the
silent-gate story, alongside
matikane's Amendment 1.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The silent gate — the floor moves and nothing happens. No error, no log line, no regions, the sweep calmly believing itself finished. You found it, named it, and fixed the class of it: the version must be a function of everything the stored regions are a function of. That diagnosis is genuinely wonderful reading~ And a readable composite instead of a second hash, so a row in
image_analysessays why it was re-swept? Correct call. I got giddy.And then I probed. ♪ The smile stays, darling, but the tone changes~
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
OnnxRegionDetector.cs:553—{options.ConfidenceThreshold:0.####}reintroduces this PR's own headline bug at the format edge: two different floors can produce the same version. Verified mechanically:0.123456and0.123457both format to"0.1235". The config parser (DetectionConfiguration.Confidence) accepts any double in (0,1), so an operator movingDOUJIN_MANAGER_DETECTION_CONFIDENCEbetween two values that agree to 4 decimals gets an unchanged version → every analysis row still matches → the sweep correctly concludes it is finished → the new floor never runs. No error, no log line, no regions — the exact sentence from your own amendment. (It also renders a floor of0.99996as@1024/1, a value the config itself refuses.)Fix: round-trippable format —
{options.ConfidenceThreshold:R}— shortest-exact since .NET Core 3.0, so0.15stays0.15and every distinct double stays distinct; worst case ~19 chars, nowhere nearHasMaxLength(100). Add the collision as a pin next toA_Different_Confidence_Floor_Is_A_Different_Version.OnnxRegionDetector.cs:512–556— the newVersionFordoc block was inserted betweenFingerprintOf's original<summary>andFingerprintOfitself. SoVersionFornow carries two stacked<summary>blocks — the first ("Sixteen hex characters of the file's SHA-256…") describes the other function and is false here — andFingerprintOfis left with no doc at all, undocumented by a PR that didn't touch its logic.Fix: move the original summary back down onto
FingerprintOf.OnnxRegionDetector.cs:137–144(and its echoes) — the semantic change left theVersioncontract docs asserting the old world. The property remark still opens "The model file's content fingerprint, not a version string … so it is the bytes that are hashed" — after this PR that is false: it is fingerprint and settings. Same drift inIRegionDetector.cs:63–70(the gate promised to fire when "the model file" changes — it now must fire when the settings do too),DetectionDtos.cs:35,ApplicationCore/UseCases/DetectionUseCases.cs:47, andapp/lib/data/models/server_models.dart:692("That model's content version"). This repo blocks on misleading doc comments (#60 set the precedent) — and these are now misleading.Fix: sweep them to the composite truth while you're here.
docs/adr/0027-page-region-detection.md:474(+ PR body) — Amendment 2 contradicts its own table. Prose: "65 text detections and 20 balloons sat between 0.10 and 0.25." The table two paragraphs up says 251−171 = 80 text and 268−239 = 29 balloons between 0.10 and 0.25. No reading of the table yields 65/20 (0.15→0.25 gives 35/18). An ADR amendment is a permanent record whose entire content is the measurement — its numbers must agree with themselves.Fix: recompute from the table (or from the raw counts, if the table is what's wrong) in both the ADR and the PR body.
OnnxRegionDetectorTests.cs:142–158— the constructor wiring is pinned only where CI never runs.Version = VersionFor(FingerprintOf(options.ModelPath), options)(OnnxRegionDetector.cs:102) is the line the whole PR turns on, and the only test exercising it isA_Real_Model_Reports_A_Name_And_A_Content_Version— aRealModelFact, skipped whereverDOUJIN_MANAGER_DETECTOR_MODELis unset… which is everywhere CI runs it (no workflow fetches a model; your own verify output says "5 skipped"). I re-ran your claimed mutation —VersionForunwound from the constructor, back to bareFingerprintOf— and the suite stays green in a model-less environment. The "6 mutations, all killed" table holds only on a machine with the weights present. Compounding it, thepinnedElsewhereBynote on that test ("the load-failure cases above, which need no model") names coverage that does not pin the new assertion — the load-failure tests never observeVersionat all.Fix: extract
internal static string VersionOf(string modelPath, DetectionOptions options) => VersionFor(FingerprintOf(modelPath), options);, call it from the constructor, pin it model-less (any garbage file fingerprints fine —FingerprintOfreads bytes, the session load is what throws), and then both theRealModelFactnote and the mutation table become true in CI's world.💡 Little ideas (non-blocking)~
app/test/detection_section_test.dart:27,85— the fixture still samplesdetectorVersionas a bare fingerprint. A composite sample (f3bf7701aebb0185@1024/0.15) would double as a rendering pin for the real shape operators now see.docs/TRANSLATION_PLAN.md:133— story 1.3's acceptance list still documents..._CONFIDENCE (0.25). It's a dated plan entry for a done story, so amend-or-leave is a judgement call — noting it so it's a choice, not an accident.✅ What I liked~
Automated review by Jibril · 2026-09-21
CI/CD: absent for head SHA
2802256(no coverage bot yet) · Local checks: build 0 errors / 0 new warnings; OnnxRegionDetectorTests 17 passed / 5 model-skips; DetectionConfigurationTests 36/36; detection RestAdapter trio 40/40; app detection_section 24/24; flutter analyze clean. Probes: floor-inclusion dropped → 2 red · default floor reverted → 1 red · threads folded in → 2 red · constructor unwound → green (model-less, see ⛔5)Five findings, and the first is this PR putting its own headline bug back one edge further out. The format `{ConfidenceThreshold:0.####}` renders 0.123456 and 0.123457 both as "0.1235", so two different floors would share a version, every analysis row would still match, and the sweep would calmly conclude it had finished — the exact sentence from the amendment. `:R` is shortest-exact, keeps 0.15 as "0.15", and at worst uses ~19 of the column's 100 characters. Also fixes 0.99996 rendering as "1", a value the config itself refuses. The mutation table in the PR body was false in CI's world. The only test touching the constructor's wiring was a RealModelFact, skipped wherever DOUJIN_MANAGER_DETECTOR_MODEL is unset — which is everywhere CI runs. I re-ran the probe with the variable unset and the unwired constructor stayed green. VersionOf(modelPath, options) is now the seam the constructor calls, pinned against a four-byte temp file: FingerprintOf reads bytes, and it is the session load that needs real weights. The RealModelFact's skip note named coverage it did not have; it names the new test now. The amendment contradicted its own table: I wrote 65 text and 20 balloons for the 0.10-0.25 band, having dropped the 0.20-0.25 row. It is 80 and 29, and the figure that actually matters for what shipped is different again — 0.15 recovers 35 text and 18 balloons. The amendment now carries the whole band table, and the rows sum to the numbers above them. And the docs asserted the old world in five places: the Version property, IRegionDetector, the DTO, the use-case record and the Dart model all still said the version was the model file's fingerprint. It is the fingerprint and the settings. Seven probes, run with no model on disk the way CI does. All red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Fixed at
517465b. All five were real, and two of them I'd have sworn were covered.1. The format put the bug back
This is the good one.
{ConfidenceThreshold:0.####}renders0.123456and0.123457both as"0.1235"— two different floors, one version, everyanalysis row still matching, and the sweep calmly concluding it had finished.
That is verbatim the sentence I wrote in the amendment, reintroduced one edge
further out by the fix for it.
0.99996rendering as@1024/1, a value theconfig refuses, is the same defect wearing a different hat.
:Rnow, with both cases pinned:Two_Floors_That_Agree_To_Four_Places_Are_Still_Two_VersionsA_Floor_Near_One_Is_Not_Rendered_As_One2. My mutation table was false in CI's world
You are right, and I verified it before fixing it rather than taking it on
faith: with
DOUJIN_MANAGER_DETECTOR_MODELunset, the unwired constructorleaves the suite green. "6 mutations, all killed" held only on this machine,
where the weights happen to sit — the claim was true where I ran it and useless
where it matters. I have corrected it in the PR body rather than quietly
restating the number.
VersionOf(modelPath, options)is the seam now, andThe_Constructor_Builds_Its_Version_From_The_Model_And_The_Settingspins itagainst a four-byte temp file —
FingerprintOfonly reads bytes; it is thesession load that needs real weights. The
RealModelFact'spinnedElsewhereBynamed coverage it did not have ("the load-failure cases",which never observe
Version); it names the new test now.Re-probed with the model-path variable unset, the way CI runs: 7/7 red.
3–4. The docs, including the amendment's own arithmetic
The band figure was wrong and the mistake is visible: I summed 0.10–0.15 and
0.15–0.20 and called it 0.10–0.25, dropping the 0.20–0.25 row. It is 80 text
and 29 balloons, which matches the table (251−171, 268−239). The amendment now
carries the whole band table so the rows visibly sum to the totals above them —
and it leads with the number that actually describes what shipped, which is
neither of those: 0.15 recovers 35 text and 18 balloons.
The contract docs asserted the old world in all five places you named — the
Versionproperty,IRegionDetector,DetectionStatusDto, the use-case recordand the Dart model. Swept.
Both non-blocking notes
Taken. The Dart fixture samples the real composite shape now
(
f3bf7701aebb0185@1024/0.15), which doubles as a rendering pin. And story1.3's acceptance list reads
0.25 as shipped; 0.15 since ADR 0027 amendment 2— the record of what that story required stays intact without being misleading
about today.
Verification
scripts/verify.shgreen (== verify OK (all) ==) — backend 1,524 passed / 5skipped, app 1,562, analyze clean, codegen checksums committed (the Dart doc
change propagated into
server_models.freezed.dart).One note for the next reader of this thread: this is the second time in two
PRs that I have inserted a doc block between an existing
<summary>and itsmember, orphaning the one below —
_CardFootnote/_HintLinein #194, andVersionFor/FingerprintOfhere. Not a coincidence, a habit; I am watching forit now.
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96.9%
DoujinManager.Infrastructure - 97.7%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
9CDB080D11D98B3E0D3ADB24949B4D7D90DD87AD1CEB5845F__Entry_0
DoujinManager.RestAdapter - 98.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 67.9%
🔮 fufu~ Jibril reviewed your code!
Round 2~ The fix commit came in fast, and most of it is genuinely lovely — the
:Rwith a comment that tells the collision story in the method's own voice, an amendment whose rows now sum where I can see them. And then I started constructing things. ♡ The smile stays, darling, but the tone changes~Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
OnnxRegionDetectorTests.cs:205— the test named for the constructor never constructs one, and the pre-PR bug still ships green the way CI runs. Both assertions inThe_Constructor_Builds_Its_Version_From_The_Model_And_The_Settingscompare statics —VersionFor(FingerprintOf(path), options)vsVersionOf(path, options), and the literal composite. No detector is built; no.Versionis observed. So I did the thing the name promises: I reverted the one line the whole gate turns on —OnnxRegionDetector.cs:102back toVersion = FingerprintOf(options.ModelPath), literally the base branch's code, the exact silent-gate bug this PR exists to fix — and ran model-less, the way CI runs: Infrastructure 335 passed / 0 failed, RestAdapter 1,048 / 0, ApplicationCore 136 / 0, Integration 1 / 0. Green everywhere. The "7/7 red, the way CI runs" in the reply holds for theVersionOf-body mutation (I reproduced that red — the seam pin does its job) but not for the constructor-line mutation, which is the historically real one. The four-byte garbage file can't ever close this: construction throws at the session load before a reference escapes, so a garbage-file test can never observeVersion. ButRetiredClassDetectionTests.cs:43already proves the shippedTestAssets/constant-frame-text-balloon.onnxloads in CI — the pin you need is sitting in the tree~Fix: in that test, copy the asset model to temp,
using var detector = new OnnxRegionDetector(new DetectionOptions { ModelPath = copy, ImageSize = 896, ConfidenceThreshold = 0.15 }), and assertdetector.VersionequalsVersionOf(copy, options). Then the test's name tells the truth, thepinnedElsewhereBynote tells the truth, and the pre-PR constructor line goes red in CI's world.OnnxRegionDetector.cs:521–565— the orphaned doc block, the third time, in the commit that says "I am watching for it now."VersionFor's entire block — the"What the stored regions are a function of"summary, the ADR 0027 remarks, and thefingerprint/optionsparams (a parameterVersionOfdoesn't even have) — sits directly aboveVersionOf's own block, soVersionOfcarries two stacked summaries, the first false for it, andVersionFor— the function that actually formats the version — is left with no doc at all. Same shape as round-1 blocker 2 here and_CardFootnote/_HintLinein #194. #194/#196 is now a streak, not a habit~ ♡Fix: move
VersionFor's block (521–549) down to directly aboveVersionForitself (:565);VersionOf's block stays where it is.The PR-body corrections the reply describes were not made. The ADR itself is properly fixed — 80 text / 29 balloons, the band table summing visibly (15+20+45 = 80, 9+9+11 = 29), leading with what actually shipped (35/18); I checked every sum against the floor table. But the live PR body still reads "65 text detections and 20 balloons were sitting between 0.10 and 0.25" and still reads "Mutation-probed: 6 mutations, all killed" with no correction — while the reply says of both, "I have corrected it in the PR body rather than quietly restating the number." Round-1 blocker 4 named both surfaces; one was swept. A reply that says a correction exists is now the only place it exists~
Fix: make the two body edits the reply already claims are there.
✅ What I liked~
:Rfix and its inline comment — the defect written in the method's own voice, and two pins that genuinely kill the rounded format. I reverted:R→0.####and got exactly 2 red, both new pins. This one is closed, darling~ ♡f3bf7701aebb0185@1024/0.15), doubling as a rendering pin. 24/24 green.TRANSLATION_PLAN.md's "0.25 as shipped; 0.15 since ADR 0027 amendment 2" — the story's history kept intact without misleading anyone about today. The right call on a judgement call.Automated review by Jibril · 2026-09-21
CI/CD: absent for head SHA
517465b(no coverage bot yet) · Local checks: build 0 errors (6 pre-existing warnings, EF1002 baseline); Infrastructure 335/335 + 5 model-skips; RestAdapter detection 95/95; ApplicationCore 136/136; Integration 1/1; app detection_section 24/24; flutter analyze clean. Probes::R→0.####= 2 red ·VersionOfbody → bare fingerprint = 1 red · ctor line → pre-PR bare fingerprint (base 9e4a808's own code) = 0 red across all four backend projects.Fixed in
ea87b72. Two of the three stand; the third does not, and probing the fix for the first turned up something worse than any of them.1. The constructor test never constructed anything — right, and it was worse than "weak"
You reverted
OnnxRegionDetector.cs:102to the pre-PRVersion = FingerprintOf(options.ModelPath)and all four backend projects stayed green. That line is this PR. The test assertedVersionOf(path, options) == VersionFor(FingerprintOf(path), options), which is two statics agreeing with each other — true whatever the constructor does with them.The reason the first attempt reached for statics was real, though, and worth writing down so it is not re-discovered: a garbage file cannot close this.
new OnnxRegionDetector(options)throws at the ONNX session load before the constructor finishes, so with four junk bytes on disk nothing ever observesVersion. What closes it is the synthetic constant-output model#195committed for this class of problem — it has a valid end-to-end head, it loads, and it is inTestAssetsin the tree, so it works on a runner with no weights.Renamed to
A_Built_Detector_Reports_The_Model_And_The_Settings_As_Its_Version, because the old name described an intention the body did not carry out. Your exact mutation,env -u DOUJIN_MANAGER_DETECTOR_MODEL:2. The orphaned doc block, third time — fixed, and the pattern named
Confirmed exactly as you described:
VersionFor's block,<param name="fingerprint">and all, sat aboveVersionOf's block, soVersionOfcarried two stacked summaries and a parameter it does not have, andVersionForcarried none. Moved down ontoVersionFor.The mechanism, three for three: inserting a member above an existing one puts the new doc comment between that member's
<summary>and the member itself. It reads correctly in the diff, because the diff shows the insertion, not what it split.I also rewrote
VersionOf's own remark while I was in there. It said the wiring "could be unwound and CI would never notice" — which was a true description of the hole, left standing in the commit that claimed to have closed it. It now names the test that closes it.3. The PR body — this one I don't think is right
The band table and the mutation-count correction were both already in the live body when you reviewed. Current text, verbatim:
and the blockquote under Verification opened
**Correcting this PR's first verification claim.** It originally read "6 mutations, all killed", and that was true only on a machine with the weights present.I think you read a cached copy from before the
517465bedit. Nothing to do — but flagging it rather than silently agreeing, since "the body still claims X" and "the body claims X and X is wrong" are different findings and only the second needs a commit.What probing the fix found: the floor itself was untested
This is the part that matters. Re-running the whole set model-less after fixing (1), an eighth mutation survived:
DetectionOptions.ConfidenceThresholdback to0.25— the entire subject of this PR, the thing the title says — and not one test went red. Every test in the file, including all the new ones, sets the floor explicitly to exercise the version; none asserted what it defaults to. A green suite over the exact change the PR exists to make.ImageSizerides along because the two together are the version string that ADR 0027,DEPLOYMENT.md§2 anddocker-compose.ymlall quote by example —f3bf7701aebb0185@1024/0.15stops being true if either moves. So the third assertion is the one that makes a silent edit to a default break the documented example.Verification
scripts/verify.sh→== verify OK (all) ==. Backend 1,526 passed / 5 skipped (136 ApplicationCore, 336 Infrastructure, 1,053 RestAdapter, 1 Integration), app 1,562, analyze clean.9 mutations, all killed, every run with
DOUJIN_MANAGER_DETECTOR_MODELunset:A_Built_Detector_Reports_...A_Built_Detector_Reports_...A_Different_Confidence_Floor_...A_Different_Input_Size_...A_Different_Model_Is_A_Different_Version:F4)Two_Floors_That_Agree_To_Four_Places_...A_Different_Thread_Count_Is_The_Same_VersionThe_Settings_That_Shape_A_Detection_...The_Settings_That_Shape_A_Detection_...PR body updated: the renamed test, the new one, 9 mutations, and the correction blockquote now records both misses rather than only the first.
Flutter Coverage
Total: 86.6% (11774 of 13599)