Fix yolov2 support and other bugs found in deep review - #131
Merged
Conversation
Merged
IanButterworth
force-pushed
the
review-fixes
branch
from
August 5, 2026 20:18
343429f to
7946b11
Compare
IanButterworth
marked this pull request as draft
August 5, 2026 20:19
IanButterworth
force-pushed
the
review-fixes
branch
from
August 5, 2026 20:42
7946b11 to
ba0d761
Compare
overridecfg! built its layer-symbol list from `cfgchanges` instead of `cfgvec`, then used indices from that list to index `cfgvec`. This only worked for `(:net, 1, ...)` changes by coincidence (`:net` is block 1 in both lists). Any other target, e.g. `(:yolo, 1, :classes, n)` as documented in the README, errored or edited the wrong block. Adds a regression test overriding a :yolo block field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reorg (passthrough) layer used by YOLOv2 was broken three ways: - `w // stride` produced Rational dims, which reshape rejects (MethodError on any use) - the batch dimension was dropped from the reshape - the constructor pushed `ch[end]` unchanged instead of `ch[end] * stride^2`, so every subsequent conv layer read weights with the wrong input-channel count. For yolov2-COCO this offset all remaining reads by exactly 3*3*(1280-1088)*1024 floats = 7,077,888 bytes -- the precise deficit in the long-standing skip note "Read 196856372 bytes. Filesize 203934260". The new implementation replicates darknet's legacy [reorg] layer exactly (reorg_cpu with forward=0, as dispatched by forward_reorg_old_layer for reverse=0 -- AlexeyAB maps [reorg] to REORG_OLD), expressed as reshape/permutedims so it stays GPU-friendly. Verified element-for-element against a literal transliteration of the C loop over multiple shapes, strides and batch sizes. With this (plus the BN weight-order fix in the next commit) yolov2-COCO loads with every weight byte consumed and detects the expected objects in the test image. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
readweights swapped to a scales-then-biases order for weight files with a darknet version header below 0.2 (yolov2/yolov2-tiny, header 0.1.0). Neither PJReddie darknet nor the AlexeyAB fork ever wrote that order: load_convolutional_weights writes biases, scales, means, vars in every version. The swap loaded BN scales as biases and vice versa, blowing up activations (layer outputs in the thousands) and producing garbage detections for the v2 models -- the likely cause of the "Figure out why results differ" skip for v2_tiny_COCO. The version header still controls the seen/seen_images field width, which is the only real pre/post-0.2 format difference. Verified: v2_COCO and v2_tiny_COCO now detect car/dog/bicycle in dog-cycle-car.png with confidences 0.69-0.86 and plausible boxes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Darknet's region layer applies logistic to objectness but softmax over the class scores (forward_region_layer), whereas the yolo-layer path applies logistic to both. ObjectDetector applied sigmoid unconditionally, so v2/v2_tiny class confidences deviated from darknet by up to ~0.15. With this, v2_COCO and v2_tiny_COCO detections match Darknet.jl (AlexeyAB) exactly to 3 decimals on the test image, for both boxes and confidences. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With the reorg, batchnorm-order and region-softmax fixes both v2 models now match Darknet.jl exactly, so remove them from the skip list. - Regenerate v2_tiny_COCO detection refs and reference images (the old ones were produced with the batchnorm scale/bias swap bug) - Add reference images and detection refs for v2_COCO (never had any) - Reformat resrefs.jl to one entry per line for reviewable diffs Parity vs Darknet.jl (AlexeyAB) verified locally for both models on both test images: identical classes, boxes and confidences within 3 decimals at detect_thresh=0.5, overlap_thresh=0.5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kern_genbools guarded with `col < cols` (strict), so the final column's flag was never set and the last candidate detection was silently discarded on every GPU inference. The sibling kernel kern_clipdetect already uses the inclusive bound. Not exercised by CI (no GPU runners); found by review, untested on CUDA hardware. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two CPU/GPU inconsistencies in CUDAExt: - kern_clipdetect kept detections with score > conf while the CPU clipdetect! keeps >= conf, so a detection exactly at threshold survived on CPU but was zeroed on GPU - kern_findmax! initialized the running max to 0 and the index to 0, so when every class score was <= 0 it reported class index 0 - idst + 1 = -5 (CPU reports 1), which would throw downstream in draw_boxes. It now seeds from the first candidate like Base.findmax, which also makes ties resolve to the first maximum Not exercised by CI (no GPU runners); found by review, untested on CUDA hardware. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`weights[:, :, a+3, outnr, :] .= outnr` indexed the anchor dimension with the output-layer number, which would BoundsError for any model with more yolo heads than anchors per head. The value was also dead: findmax! subsequently writes the best class index into row a+3 (end-1) for every column, so the output number never reached the results. Output is byte-identical with the write removed. Also correct the comment describing what the 4 extra attribute rows actually contain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The :soft branch decayed a local scores array that never fed back into the detections, then set write_idx = idx_len - 1 without shifting the candidate window, so: - every box was kept with its original score (no suppression effect) - the already-kept box stayed at position 1 of the working index list, so it could be pushed to `keep` again while the true tail was dropped Now decayed scores are written back into dets (row end-2), remaining candidates are re-sorted by decayed score, and the window advances correctly. Per the Soft-NMS gaussian formulation no box is removed; pruning is the caller's score threshold's job. Documented the mutation in the docstring. Verified on a synthetic case: overlapped box decays 0.8 -> 0.37, disjoint box unchanged, no duplicate indices; :default path unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch for a 2D Float32 image whose (transposed) size matches the destination returned a bare 1-channel array instead of the `(array, padding)` tuple every other branch returns, breaking the documented `arr, padding = prepare_image!(...)` destructuring, and did not expand to the destination's channel count. It now repeats to `size(dest_arr, 3)` channels and returns zero padding, matching the equivalent branch in `prepare_image`. Adds a direct regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The `@static if flux_maxpool` toggle's else-branch was unreachable and broken: its _maxpool closure called `darknet_maxpool_layer`, a function that does not exist anywhere (the implementation below it is named `maxpool`), so flipping the flag could never have worked. Keep the Flux implementation, drop the toggle and the ~75 dead lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- `disable_bumper` does not exist; the kwarg is `disallow_bumper` (copy-pasting the example errored) - v2_COCO is no longer broken (the weights were fine; the reorg layer and batchnorm read order were the bugs), and all supported models now have tested darknet parity, not just v3+ - fix "accelleration" typo and a comment describing v3_608 as "YOLOv3-tiny" Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Thresholding happens in clipdetect! before NMS and was never repeated, so soft-NMS could return boxes whose decayed score had fallen below the requested detection threshold (e.g. two identical boxes at 0.9/0.8 with detect_thresh=0.5 returned scoring 0.9 and 0.15). perform_detection_nms now takes detect_thresh and prunes kept boxes below it; a no-op for the hard-suppression kinds, whose survivors always already passed the threshold. Adds NMS unit tests. Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Darknet's region layer applies softmax to class scores only when the softmax flag is enabled (parser.c defaults it to 0; forward_region_layer otherwise leaves the class scores linear). The bundled yolov2 cfgs set softmax=1 so their behavior is unchanged, but custom region cfgs with softmax=0 no longer get incorrectly normalized probabilities. Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
findmax! (CPU and CUDA) scanned class scores through end-3, which includes the first zero-initialized scratch attribute appended after the classes. With all-negative class scores (possible with region softmax=0 or yolo heads without logistic activation) the zero scratch row would win, reporting a class index one past the last real class. Scan through end-4 (the last real class) on both paths. No output change for the bundled models (their class scores are post-sigmoid/softmax and strictly positive); verified v2, v2_tiny, v3_tiny and v3 still match the stored reference results to 1e-5. Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mutation was documented but invisible in the name; the bang makes it explicit for any external caller. Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the per-round sort of a freshly allocated index slice with an in-place compact-and-swap: only the round's argmax needs to reach the front for the keep order to be identical. O(N) per round, no allocations, matching the file's allocation-free intent. Fuzz-verified against a full-sort reference implementation on 200 random cases (identical keep order and decayed scores). Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A non-conforming model now gets a clear assertion message instead of a confusing downstream reshape error, and the docstring says "by a factor of stride" rather than "by stride". Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reuse the class-score view (courtesy of the enclosing @views block) and apply exp and normalization in place instead of materializing intermediate arrays. v2 outputs verified unchanged vs stored refs. Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The one-entry-per-line format put multi-KB matrices on single lines, so any single-value change rewrote the whole line -- the opposite of diffable. Restore the original row-per-line layout (single-column matrices stay inline since a bare column would parse as a Vector). Verified bit-identical to the previous values after round-trip. Addresses PR review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Region softmax is gated on the cfg flag, soft-NMS prunes decayed boxes below detect_thresh automatically, and the CPU findmax scratch-row fix gets its own bullet as a user-visible CPU behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
IanButterworth
force-pushed
the
review-fixes
branch
from
August 5, 2026 20:51
ba0d761 to
b97f4c4
Compare
@artifact_str locates its Artifacts.toml by walking up from the calling file's directory, and find_artifacts_toml stops at the first directory containing a Project.toml. The new test/Project.toml therefore ended the search before it reached the repo-root Artifacts.toml, breaking the "Download all artifacts" testset on CI. Download the weights via YOLO_MODELS instead, which resolves artifacts from inside the package where the lookup works, covers future models without editing the list, and lets the now-unused LazyArtifacts test dependency be dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
IanButterworth
marked this pull request as ready for review
August 6, 2026 14:07
IanButterworth
commented
Aug 6, 2026
Comment on lines
+31
to
+32
| [sources] | ||
| Functors = {rev = "master", url = "https://github.com/FluxML/Functors.jl"} |
Collaborator
Author
There was a problem hiding this comment.
Remove this and the direct dep before merge
Collaborator
Author
There was a problem hiding this comment.
I want to keep iterating, so I'm going to merge and remove this before release
IanButterworth
added a commit
that referenced
this pull request
Aug 7, 2026
…r deps (#138) * Remove unused flipdict and createcountdict helpers Neither has any call site in src, test, or examples. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Remove unused lhtan_grad activation gradient Gradient kernels are training-only; the package does no training and nothing references it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Remove dead cu_functional guard cu_functional() held a CUDA.allowscalar(false) call but was never invoked, so it never ran. allowscalar is a global CUDA.jl session flag, so a package should not impose it on load either -- scalar-indexing policy belongs to the user. Delete the dead function and its Ref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Remove pre-1.9 extension fallback shim Base.get_extension exists since Julia 1.9 and the package requires 1.10, so the include branch was unreachable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Drop redundant using statements in prepareimage.jl The including module already imports these. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Parameterize AllocWrappedModel Concretely-typed fields avoid dynamic dispatch on every call through the wrapper, which is the default CPU entry path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Keep weights-file training metadata instead of discarding it seen/seen_images were read (necessarily, to advance the stream) and then dropped; store them in cfg where darknetversion already lives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Key benchmark() by model name and cover all models The hardcoded constructor list with numeric select indices was stale (none of the six new models) and fragile. Iterate YOLO_MODELS keys instead; each model loads at its native default size. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix benchmark table printing under PrettyTables v3 Compat allows PrettyTables 2 and 3, but v3 renamed the header kwarg to column_labels, so benchmark()'s final table throw a MethodError on any v3 resolve. Gate on pkgversion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Use Base.identity for the linear activation linear(x) = x duplicated identity; the codebase already used identity elsewhere for the same purpose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Validate input dims against the network's actual maximum stride assertdimconform required width/height divisible by the FIRST CONV'S FILTER COUNT -- an output-channel number with no relation to spatial downsampling. It worked for the classic models only because their first conv happens to have 32 (or 16) filters, and it wrongly rejected valid sizes (yolov7x at 416, first conv filters=40). Compute the real constraint instead: walk the cfg blocks tracking each layer's cumulative downsample (conv/maxpool multiply by stride, upsample divides, reorg multiplies, routes adopt the referenced layer's scale) and require divisibility by the maximum. Verified to give 32 for all classic models and 64 for v4_p6, matching their head strides; v7x now constructs at 416 and invalid sizes still throw. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Extract shared draw_boxes geometry helper Both draw_boxes! variants recomputed the identical image/model ratio and coordinate-index mapping; factor it into _box_geometry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add a precompile workload PrecompileTools was a dependency with no workload. Use the (recently fixed) dummy-weight path to compile cfg parsing, model construction and the full inference + NMS pipeline at precompile time: time-to-first- inference in a fresh session drops from ~12.8s to ~0.7s on an M2 Pro (package load ~3.8s warm). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move benchmark() behind a BenchmarkTools/PrettyTables extension BenchmarkTools and PrettyTables were hard dependencies used only by the benchmark() utility, taxing load time for every detection-only user. They are now weakdeps triggering a BenchmarkExt extension; calling benchmark() without them loaded raises a MethodError with a hint explaining what to load. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Narrow Flux compat to tested versions 0.12/0.13 predate the extension mechanism this package relies on and have never been exercised by CI; advertising them is risk without evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail on missing detection references instead of self-blessing get!(RES_REFS, key, computed) meant a new model/image combination passed trivially on its first run without comparing anything. Missing keys are now test failures, with the regeneration script referenced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Commit the test-reference generation script The script that produced resrefs.jl and the reference images lived outside the repo; anyone needing to regenerate references (new model, intentional output change) had to reconstruct it. It regenerates any subset of models, asserts darknet parity for every model/image combination before blessing, and uses the same reduced test sizes as the suite. Round-trip verified on v3_tiny (max ref drift 9e-7, within the suite's 0.05 tolerance). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add changelog entries for quality pass Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Document remaining review findings in dev/REVIEW_BACKLOG.md Everything raised across the bug/performance/model-coverage/quality review series that was not fixed in #131/#132/#137/#138, with effort estimates and the explicitly-rejected items recorded so the reasoning is not lost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Merged
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.
Note
This PR was written by Claude (Claude Code), from a deep review of the repo requested and supervised by @IanButterworth.
This PR fixes every confirmed bug from a full-repo review, one fix per commit.
Headline: yolov2 models fixed and re-enabled
v2_COCO(skipped: "Not all weights are read during load") andv2_tiny_COCO(skipped: "results differ") were broken by three stacked bugs:w // strideproducedRationaldims whichreshaperejects; the batch dim was dropped; and the constructor didn't multiply channels bystride², so every conv after the reorg read weights with the wrong input-channel count. For yolov2-COCO the resulting deficit is exactly3*3*(1280-1088)*1024floats = 7,077,888 bytes — precisely the gap in the old skip note ("Read 196856372 bytes. Filesize 203934260"). The weights were never bad. The new implementation replicates darknet's legacy[reorg](reorg_cpuwithforward=0, dispatched viaREORG_OLDin AlexeyAB) as reshape/permutedims, was verified element-for-element against a literal transliteration of the C loop, and asserts input divisibility so non-conforming models get a clear error.load_convolutional_weights: biases, scales, means, vars in every version), so BN scales/biases were swapped, blowing up activations. The version header still governs theseenfield width, which is the only real format difference.softmax=1(as the bundled v2 cfgs do; withsoftmax=0, class scores are left linear, matchingforward_region_layer). Sigmoid was applied to both unconditionally, skewing v2 confidences by up to ~0.15.With all three fixed, both v2 models match Darknet.jl (AlexeyAB) exactly to 3 decimals on both test images (classes, boxes, confidences; verified locally on macOS). They're removed from the skip list, their artifacts download in tests again, and reference images/detection refs are regenerated (the old v2_tiny refs were produced under the BN bug).
Other fixes
overridecfg!indexedcfgvecwith indices computed fromcfgchanges— only(:net, 1, ...)worked, by coincidence. The documented(:yolo, 1, :classes, n)use case errored. One-line fix + regression test.keepdetectionsalways dropped the last detection —col < cols(strict) inkern_genboolsmeant the final column's flag was never set.kern_clipdetectused>where CPU keeps>=;kern_findmax!seeded its max at 0/index 0, reporting an invalid class index when all scores are ≤ 0.softmax=0) selected a phantom class one past the last real one.nmsis renamednms!accordingly), survivors are selected allocation-free, and kept boxes whose decayed score falls belowdetect_threshare pruned. Unit-tested, plus fuzz-verified against a reference implementation.outnrwrite with latent BoundsError —weights[:, :, a+3, outnr, :]indexed the anchor dim with the output number (BoundsError for models with more heads than anchors), and the value was overwritten byfindmax!anyway. Removed; output is byte-identical.prepare_image!matching-size 2DFloat32branch returned a bare 1-channel array instead of the(arr, padding)tuple, breaking the documented destructuring. Fixed + test.@static if flux_maxpoolelse-branch calleddarknet_maxpool_layer, which doesn't exist anywhere; flipping the flag could never have worked (−77 lines).disable_bumper→disallow_bumper(the documented kwarg didn't exist), v2 no longer marked broken, typo fixes.All bundled models (v2, v2_tiny, v3, v3_tiny) were re-verified against the stored reference results (1e-5) after the follow-up commits.
Caveats
🤖 Generated with Claude Code