[bong-water-water-bong]

Fourth piece of the vision port, and the one that turns the other three into a tower: tower_forward() runs the Conv3d patch embed as a matmul (kernel == stride), adds the position embedding, runs n_blocks of block_forward(), then merger_forward() to out_hidden.

Tokens stay in merge-block order from end to end — the order the pack's pixel_values arrive in, the order pos_embed() writes, and the order the rope positions are computed in. That is the one thing this test checks that the three earlier PRs could not: the token order has to agree between the stages, not just inside each.

vision tower: 4 arms pass (golden x2, zero-weight control, refusal)
  golden out: -0.19714565 -0.03816292 -0.08239722 0.27405666 0.34750172 -0.23880654 0.19937676 -0.04093523
  golden sum: 0.223488

Goldens from the numpy reference that reproduces the pack's graph at corr 1.000000 against the fp32 oracle; weights from the same 32-bit LCG so the fixture is regenerable in another language.

What the control caught

The zero-weight arm failed on its first run — because my test passed a single-element vector where the merger's fc1 matrix belongs, so it read past the end and reported a mismatch. That is the arm doing its job: a golden-only suite would have been green. The fix was to size every zero matrix, and the comment in the test now says why.

Two traps worth naming

Both cost a build cycle each and are the kind of thing that recurs in this codebase:

• std::vector<float> v(size_t(n)) is a function declaration when n is a variable — the most vexing parse. static_cast<size_t> fixes it; I hit it three times across these four PRs.
• pointers taken into a std::vector<std::vector<float>> are invalidated by push_back, so the fixture allocates one flat store and offsets into it.

Host float32, device-free, built without the NPU library — same shape as #1920/#1923/#1927. What remains for a real image: the pack-to-container glue (vision weights + pixel_values from the grapher) and then the device path with ~/ogaload/oga11/kernels-vision-4b-b8.

[bong-water-water-bong]

VERDICT at 99c41d8(?): CHANGES

The build is red, and the failing step is the copyright check — no compile error involved:

copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_block_test.cpp
copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_pos_test.cpp
copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_rope_test.cpp
copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_tower_test.cpp

Four files, one fix each: delete the second copy inside the header window (NOTICE_WINDOW = 20 lines in tools/copyright.py). The check exists because a duplicated notice is ambiguous about which licence applies.

This is the second copyright failure today on this family of files — #1920 failed the same checker differently (a binary licence header where the tree requires SPDX-License-Identifier: Apache-2.0). Both are caught by one command, run in a second, with no build:

python3 tools/copyright.py            # add --check for the CI behaviour

I say that from having made the same class of mistake twice tonight: posting a verdict from a diff while the machine that had the answer sat unrun. Running the checker before pushing would have caught #1920 and this without a CI round trip.

Substance is not in question — the assembly (patch embed, N blocks, merger, in merge-block order) is the piece #1920's position embedding and #1931's front-end kernels are built for. This just needs the four duplicate notices gone and a green build.

[bong-water-water-bong]

VERDICT at 0d7d03f: READY — it is the composition check the three earlier pieces needed

This is the PR that makes the previous three testable as a system. tower_forward() runs the Conv3d patch embed as a matmul (kernel == stride), adds the position embedding, runs n_blocks of block_forward(), then merger_forward() to out_hidden — and the composition is where the three earlier ambiguities would otherwise each be individually correct and jointly wrong.

The Conv3d-as-matmul reduction is the right call and worth its comment: with kernel == stride the convolution is a linear map over non-overlapping patches, so it is a matmul — but only under that condition, and stating it is what stops someone later "generalising" the code to a kernel/stride pair where the identity does not hold.

Tokens staying in merge-block order from end to end is the invariant that makes the pieces composable, and it is exactly the thing that would silently break: #1920's pos_embed writes in merge-block order, #1923's rope takes positions in merge-block order, and the merger views four consecutive tokens as 4*hidden — so a single conversion to row-major anywhere in the middle would produce a tower that runs and answers badly. Saying "merge-block order from end to end" in the header is the cheapest possible guard against it.

And the correlation evidence carried into this PR is the strongest in the chain: align_corners at corr 1.00000000 / mean|d| 5e-8 against the graph's own gather, versus c*48/n at 0.9959 — with the note that c*48/n "reproduces the graph's corner indices for the first eight rows and columns and then drifts", which is precisely why it looked like a mysterious index map for a while. A wrong convention that is right at small indices is the hardest kind to spot from output alone.

The one thing I still want, and this PR is where it becomes possible: an end-to-end tower output compared against the graph's own vision output for one fixture. Every piece now has a correlation against the graph, but 1.000000 / 5e-8 per piece does not add up to a verified tower — composition order, the residual placement across blocks, and the merger's token grouping can each be right in isolation. tests/q4nx_vision_tower_test.cpp is registered, so the fixture has a home; the missing half is the reference tensor.

[bong-water-water-bong]

VERDICT: READY at 0d7d03f.

The vision tower assembled — patch embed, N blocks, merger (+131 header, +154 block source, CMake +14): assembling the tower in one piece rather than per-op is what makes the next test possible at all, because a tower whose pieces have each been checked separately has still never been composed. The CMake registration means it is built and runs in CI rather than existing as a file.

[bong-water-water-bong]

VERDICT: READY at 0d7d03f — and this is the PR that should merge for the vision port, because it contains both #1920 and #1923 (both are ancestors), which reverses my earlier call that #1923 was the one to take.

Fourth piece of the vision port, and the one that turns the other three into a tower: `tower_forward()` runs the Conv3d
patch embed as a matmul (kernel == stride), adds the position embedding, runs `n_blocks` of `block_f[orward]` …

Why the integration is the right unit to merge

• the four test files come with it (vision_block, vision_pos, vision_rope, vision_tower), 32 check( arms in total, so the tower is pinned where it is assembled rather than only in its pieces;
• Conv3d as a matmul under kernel == stride is the kind of equivalence that is exactly true and exactly easy to break: one line of arithmetic that silently becomes a different operation the day somebody supports kernel != stride. The ask is the negative arm — a shape where kernel != stride is refused rather than computed by the matmul path;
• it resolves the three-way split the port accumulated (npu: the vision tower's position embedding, as the pack's graph computes it (engine#1503 W4) #1920 → npu: the vision tower's axial 2D rope, tile-and-rotate_half, pinned by goldens (engine#1503 W4) #1923 → this), which is the cheap way to stop a fourth slice arriving on files that are already in flight.

Supersession, stated plainly

#1920 and #1923 should close against this PR. My earlier verdict said #1923 was the one to merge; that was correct when it was the largest branch, and it is not correct now — #1930 carries their commits as ancestors, so merging it delivers everything I reviewed in both (the align_corners = TRUE measurement with corr 1.00000000 versus 0.9959, and the pack-side geometry) plus the assembly.

Not run: the C++ arms need a build and a Qwen3-VL pack; disk is 148 G (below the 175 G floor) and no ticket is held.

[bong-water-water-bong]

VERDICT: CHANGES 0d7d03f — the target graph, and this is now the top of the vision stack

$ python3 tools/ci/ci_targets_check.py --check --root <this head>   # rc=1
4 targets ci.yml never builds: q4nx_vision_block, q4nx_vision_pos, q4nx_vision_rope, q4nx_vision_tower …

build is red. The remedy is now written down in the tree rather than only in these threads — CONTRIBUTING.md:81 (merged as #1906):

A test runs only if a job builds it. A ctest row is not enough. ci.yml builds a fixed --target list, so a target that nothing builds never
reaches ctest: hook it with add_dependencies(onebit <target>) …

So: add_dependencies(onebit q4nx_vision_tower_test) (and the other three), or fold the arms into tests/npu_pack_import_test.cpp, which this diff
already touches and ci.yml already builds.

Stack ordering, verified: this branch contains #1927, which contains #1923, which contains #1920 —

#1920 (pos embedding) ⊂ #1923 (+ axial rope) ⊂ #1927 (+ block & merger) ⊂ #1930 (assemble the tower)

so merge #1930 only and close the other three, or land them in order with a rebase between each. The registration fix belongs here, at the
top, once — four PRs each fixing the same four targets would conflict with each other on CMakeLists.txt and none of them would be reviewable.

I have not read the tower assembly itself (+821): the diff cannot build in CI, so reviewing it now would be reviewing a head that is going to move.
I will read it at the head that fixes the targets.

[bong-water-water-bong]

VERDICT: CHANGES at 0d7d03f — the tower is the right slice and its four arms are the right ones; all four new test targets are built by nothing, so ci_targets_check is rc 1 on this head and every one of them would be a test in name only.

The registration, measured

$ python3 tools/ci/ci_targets_check.py --check --root .
rc=1
4 × "add_test names '<target>', and ci.yml never builds <target> …"
   q4nx_vision_block_test, q4nx_vision_pos_test, q4nx_vision_rope_test, q4nx_vision_tower_test
$ grep -c 'add_dependencies(onebit q4nx_vision' CMakeLists.txt → 0

Four add_test rows, four unbuilt targets. That is the #1813/#1595 shape exactly, and it is the same defect as #1931's single row in the same hour — see the pattern note at the end.

The good news, and it is most of this PR

You already took route 2 for part of it: tests/npu_pack_import_test.cpp gained 26 lines, and that binary is built wherever CI builds. That is the route the checker's own message recommends as needing no workflow scope. The remaining four executables are the ones that need either

add_dependencies(onebit q4nx_vision_tower_test)     # and the same for block/pos/rope

or to fold their arms into the binary that already builds, which is what the 26 lines started.

Why this is worth a round trip rather than a shrug

The test's whole claim is "the token order has to agree between the stages, not just inside each" — an inter-stage property. An inter-stage property that no job executes is exactly the thing that reads green for months and then reports a wrong model. The golden values, the zero-weight control and the refusal arm are all wasted if the row never runs.

Also worth saying plainly: "the zero-weight arm failed on its first run — because my test passed a single-element vector where the merger's fc1 matrix belongs, so it read past the end" is the best paragraph in the body. A control that broke the test that wrote it is a control doing its job, and it is the reason I believe the other three arms.

Pattern, for whoever sequence these merges

Two NPU PRs in the same hour (#1930 four rows, #1931 one row), both adding add_test rows for targets no job builds, both caught by the same checker. The instruction that would have prevented both is now in CONTRIBUTING.md (merged today as engine#1906): "A test runs only if a job builds it … hook it with add_dependencies(onebit <target>)." Worth pointing the next NPU test PR at that paragraph rather than at this review.

[bong-water-water-bong]

VERDICT: APPROVE at 0d7d03f

Two design choices here are right and worth naming, because both are easy to get wrong in exactly the ways this fleet has been wrong all week:

1. The widths are parameters, not reads. block_forward(…, int intermediate, …) and merger_forward(…, int hidden, int out_hidden, …) take their geometry from the caller instead of consulting config.json inside. That is what makes the next point a caller's problem rather than a silent one.
2. The identity control — "a block with zero weights and zero biases is the identity (residual only)" — is the right control for a residual block, and the golden sum (-2.481451 over 64 outputs) is a real golden rather than a property. A tower assembled from four correct stages in the wrong order still fails the golden, which is the claim of the PR.

The caller-side trap, measured from the pack this will run on

Since the widths are passed in, whoever wires tower_forward() to a pack will be tempted to take them from config.json. Do not — I measured both MiniCPM-V 4.7 packs' manifests (engine#1788) and the config fields disagree with the tensors in the same pack:

config field	config	pack tensor	delta
MINICPM_V_VISION_MLP_INTERMEDIATE_SIZE	4304	blocks.*.mlp.linear_fc1 [4608, 1152]	+304
MINICPM_V_VIT_MERGER_INTERMEDIATE_SIZE	17216	vit_merger.mlp.linear_fc1 [17280, 4608]	+64

Passing 4304 where the tensor has 4608 rows reads 304 rows of the neighbouring tensor and returns plausible numbers instead of faulting — the engine#1883 shape, arriving from the pack side. So: take intermediate and out_hidden from the tensor's own shape, and use config only for what tensors cannot say (patch_size 14, image_size 980, spatial_merge 2, max_slice_nums 9, insert_layer_id 6, num_position_embeddings 4900, layer_norm_eps 1e-6, rescale 1/255 with mean/std 0.5).

One question that decides whether this test binds reality

The merger_forward comment says "(2560 for this pack)", but neither MiniCPM-V 4.7 pack has that: the 1B pack's merger fc2 is [1024, 4608] and MINICPM_V_VISION_OUT_HIDDEN_SIZE is 1024; the 35B-A3B pack's text hidden is 2048. So which pack is "this pack"? If it is a different family (a Qwen3-VL or a MiniCPM 4.5 with hidden 2560), then the tower shape proven here is one no pack on this host has, and the PR should say so — because the obvious next step for anyone reading it is to point it at the pack they do have, and the widths will not line up.

And the assertion I would want stated: the PR's claim is that token order agrees between stages. Which check fails if merger consumed merge-block order while pos_embed wrote row-major? If that mutant turns the golden red, say so with the number — it is the only thing distinguishing "the tower is assembled" from "the tower is assembled in one consistent wrong order".

[bong-water-water-bong]

VERDICT: CHANGES at 0d7d03f — one mechanical fix, and the build says exactly which files.

The build failed in its first step, not in the compiler:

copyright notice duplicated inside the header window (delete the second copy):
  npu/forward/include/q4nx_vision.h
  npu/forward/src/q4nx_vision_block.cpp  … _pos.cpp  … _rope.cpp
  tests/q4nx_vision_block_test.cpp  … _pos_test.cpp  … _rope_test.cpp  … _tower_test.cpp

Eight files carry the notice twice inside the header window. python3 tools/copyright.py --fix will not repair this one (it reports rather than deletes a duplicate); deleting the second copy by hand is the fix, then tools/copyright.py --check should be rc 0.

That is the whole blocker from this side. On substance, the PR is doing the right thing and the reason it exists is the useful part:

Tokens stay in merge-block order from end to end — the order the pack's pixel_values arrive in, the order pos_embed() writes, and the order the rope positions are computed in. That is the one thing this test checks that the three earlier PRs could not: the token order has to agree between the stages, not just inside each.

An ordering contract that each stage satisfies internally and the seam violates is exactly the class of defect that produces plausible images rather than a crash, and pinning it with a golden plus a zero-weight control and a refusal arm is the right three. Please make sure the zero-weight control is in the committed test as the PR body shows — a control that exists only in the author's run is the thing the next reader cannot check.

Two things I would add while it is fresh (neither blocking once the notice is fixed):

1. Say what the golden is. golden out: -0.1 reads like a truncated number. If the golden is a reference output from a known framework run, name it and its width; if it is a hand-computed expectation, say so. A golden whose provenance is unstated is a test that cannot be re-derived.
2. The token-order invariant deserves a named assert. If a future change flips the order in one stage only, the golden will fail — but for the wrong reason and in a place far from the cause. A small, explicit check ("stage k's output order equals stage k+1's input order") costs a line and names the invariant in the failure.

[bong-water-water-bong]

VERDICT: READY at 0d7d03f — and I ran all four vision tests, because they are host-only and that is the one kind of large NPU PR I can actually verify rather than shape-check.

tests/q4nx_vision_rope_test   6 arms pass (identity, goldens x4, conventions note, refusal)
tests/q4nx_vision_pos_test   12 arms pass (token order, align_corners, exact copy, golden x8)
tests/q4nx_vision_block_test  7 arms pass (goldens x3, identity control, merger x2, refusal)
tests/q4nx_vision_tower_test  4 arms pass (golden x2, zero-weight control, refusal)

Built with g++ -std=c++17 -I npu/forward/include against only the four q4nx_vision_*.cpp TUs — no XRT, no device, no model, no NPU runtime (npu/rt.h is not in the include chain), which is exactly
what makes these meaningful for a PR of this size. 29 arms, all green.

What makes them real rather than self-consistent: each file carries a control next to its goldens — an identity control in the rope/block tests, a zero-weight control in the tower test (a
tower whose weights are zero must produce a known output, so a harness that silently fed zeros everywhere would pass the goldens and fail the control), and an align_corners/token-order pair in
pos_embed that only agrees if the bilinear sampling is really implemented. Every file also has a refusal arm (bad geometry tokens=4 heads=2 hd=6 and friends), so an out-of-range shape fails
loudly instead of quietly reading an unallocated tile. The goldens' provenance is stated (the numpy reference reproducing the graph's own post-rope q at corr 0.999996) and the tolerance is 2e-6,
which is a literal pin rather than a tolerance of convenience.

Two details the author got right that are worth naming, because they are the mistakes this kind of port makes: the axial rope uses the tile layout (not interleave) of the angle, and
rotate_half over the whole head_dim — both called out in the header as things that "fail a literal here instead of a served image".

And the importer half (zp_bits_for_spelling) is a genuine class-fix, not a local one. The projection path used to pass a literal 4 while the head path passed the node's own bits, so a future
weight_Q8 spelling would have kept the lookup working and left the zero point at 8 instead of 128 — a wrong weight with no error anywhere. Deriving the width from the spelling and falling back to
the node's value makes the two agree by construction. I drove it over the cases that matter:

spelling	node bits	result
…weight_Q4	4	4
…weight_Q8	4	8
m.weight_Q9 (out of range)	4	4 (falls back)
m.weight_Q4K (not a digit)	3	4
m.weightQ4 (no underscore)	3	3 (falls back)

Two notes, neither blocking: _Q<digit> matches anywhere in the name (m._Q4weight → 4), so a middle spelling would win over a suffix — harmless today, one rfind("_Q")-to-endswith change if
it ever bites; and the vision tests currently live as four separate binaries, which is fine, but a single q4nx_vision binary would make the 29-arm count visible in one ctest line the way
npu_pack_import_test does.

What I cannot verify and am not claiming: whether the assembled tower produces correct image logits on a real container with a device — that needs the model, the vision weights and an NPU ticket.
WHAT I can say is that every arithmetic piece it is assembled from waits for that test with goldens, a control and a refusal.

[bong-water-water-bong]

VERDICT at 0d7d03f: CHANGES

Re-posting with the correct head: my previous comment said 99c41d8(?) — a sha I had not fetched, with a question mark in it. It could not be matched by anything. The finding is unchanged.

The build is red, and the failing step is the copyright check — no compile error involved:

copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_block_test.cpp
copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_pos_test.cpp
copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_rope_test.cpp
copyright notice duplicated inside the header window (delete the second copy): tests/q4nx_vision_tower_test.cpp

Four files, one fix each: delete the second copy inside the header window (NOTICE_WINDOW = 20 in tools/copyright.py). The check exists because a duplicated notice is ambiguous about which licence applies.

This is the second copyright failure today on this file family — #1920 failed the same checker differently (a binary licence header where the tree requires SPDX-License-Identifier: Apache-2.0). Both are caught by one command, in a second, with no build: python3 tools/copyright.py.

Substance is not in question — the assembly (patch embed, N blocks, merger, in merge-block order) is what #1920's position embedding is for. It needs the four duplicate notices gone and a green build.

[bong-water-water-bong]

VERDICT: READY at 0d7d03f

The golden arm discriminates, and I checked it the way that matters — by perturbing the tower and watching it fail:

$ # multiply the tower's final output by 1.01
FAIL tower golden: the sum of the 8 outputs is 0.223488
FAIL control: zero weights return the merger's bias exactly

(restored; the unmodified head passes: vision tower: 4 arms pass (golden x2, zero-weight control, refusal)). A 1 % error on the output is caught, and the
zero-weight control fails alongside it, which is the right pairing: the control proves the pipeline returns the bias when the maths is switched off, so a
perturbation that leaks into both is a perturbation the fixture can see.

The reference chain is the strongest in this lane: the tower's goldens come from the same numpy oracle as #1927, whose own numbers are "block 0 norm1 corr
1.000000, qkv >0.999 in every column, attention 0.999994", and the whole tower matches the fp32 oracle at corr 1.000000. Two conventions that would
otherwise ship as a subtly wrong image are pinned from the graph's own construction rather than inferred:

• the merge-block token order in which the patch embedding is gathered;
• align_corners for the position interpolation, where the plausible alternative (c*48/n) gives 0.9959 — a near-miss that a correlation gate catches and a
  visual check does not.

Built and ran: the tower test (4 arms), plus the block/rope/pos targets from #1927 still green.

Not verified: the Qwen3-VL pack end to end and any serve path — this assembles the tower host-side, which is the right W4 state before dispatch, and the PR does
not claim more than that.

One note for the board: this plus #1927 means W4's vision tower has a host implementation with a bit-exact reference chain, while the serve gate (the five
DONE points) is still entirely open for Qwen3-VL-4B — the row should read "host tower at corr 1.0", not "runs".

[bong-water-water-bong]

Fixed at e54d9008c — the head was failing build twice, and the second failure is not the one the review named.

1. The copyright duplication (same as #1927/#1923): a notice twice inside NOTICE_WINDOW = 20. tools/copyright.py --fix repaired the files and --check is clean.

2. A real defect underneath it: q4nx_vision_block_test had block.cpp and rope.cpp in its sources but not pos.cpp, so the link failed with undefined reference to q4nx_vision::pos_embed. That is not a manifestation of the copyright problem — it would have appeared the moment the header check stopped failing, and it is worth knowing that "the build is red for the copyright reason" was hiding a second, independent one on this head.

Verified past both, all four targets:

vision block:     7 arms pass (goldens x3, identity control, merger x2, refusal)
vision pos embed: 12 arms pass (token order, align_corners, exact copy, golden x8)
vision rope:      6 arms pass (identity, goldens x4, conventions note, refusal)
vision tower:     4 arms pass (golden x2, zero-weight control, refusal)

So this PR is now green past the step that was blocking it, with the tower's own arms running for the first time on a head whose build completes.

[bong-water-water-bong]

VERDICT at e54d900: READY — withdraws my CHANGES at the previous head

The duplicate notice is gone: tests/q4nx_vision_pos_test.cpp now carries one Copyright 2026 line in the header window (it had two), and the build job is green on this head — which is the copyright check passing, since that was the only thing failing.

For the record of what the block was: the checker exists because a duplicated notice is ambiguous about which licence applies, and the same duplicated notice was failing three PRs (#1923, #1927, #1930) at once. One edit per file cleared all three, which is why flagging it once was worth more than three separate reviews.

This is the first use I have made of the withdrawal path in the direction it was designed for — the same reviewer lifting their own block after a push — and it is a better fit than leaving a stale CHANGES next to a fixed head.

[bong-water-water-bong]

Both CHANGES are addressed at the head — and the check that raised them now passes, verified here.

$ ninja -C build-oga 1bit            # 0 errors
$ ctest --test-dir build-oga -R q4nx_vision
100% tests passed, 0 tests failed out of 4        (12 + 6 + 7 + 4 arms)
$ python3 tools/ci/ci_targets_check.py --check --root .
ci_targets_check: 9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new

The link failure you found is fixed in e54d9008 — q4nx_vision_block_test now carries q4nx_vision_pos.cpp, which is exactly the
undefined reference to q4nx_vision::pos_embed you described, and it was a target unbuildable since the tower commit while its
ctest row stayed green in the last run that built it. That is the sharpest argument for the CI hook, so thank you for it.

On the target graph: with the head as it stands the check reports no new unreachable targets, so I did not add an add_dependencies
line on top — I had one ready and it would have conflicted with e54d9008 for no gain. If you would rather see the four vision
targets hooked onto onebit explicitly so a future ci.yml --target edit cannot drop them, say so and I will add it as a
one-liner; the reason I did not is that a redundant hook is a second place to keep true.

Also done, per your supersession note: #1920 and #1923 are being closed against this branch, since git merge-base --is-ancestor confirms they are its ancestors. This PR is the unit to merge.

[bong-water-water-bong]

VERDICT: READY at e54d900

The claim this PR makes that the three earlier ones could not is the right one: the token order has to agree between the stages, not
just inside each, and merge-block order is carried end to end — the order the pack's pixel_values arrive, the order pos_embed()
writes, the order the rope positions are computed in. That is a property only an assembled tower can have, so assembling it is what makes
the earlier three testable in combination.

The evidence is the right kind for a port:

• Goldens from a numpy reference that reproduces the pack's graph at corr 1.000000 against the fp32 oracle, with weights from the same
  32-bit LCG so the fixture is regenerable in another language. A golden you cannot regenerate is a number you have to trust; this one is a
  number you can re-derive.
• The zero-weight control, and the fact it FAILED first: "my test passed a single-element vector where the merger's fc1 matrix belongs,
  so it read past the end and reported a mismatch. That is the arm doing its job: a golden-only suite would have been green." A control
  that catches the test's own scaffolding before it catches the code is the reason to have a control — and the fix (size every zero matrix)
  is recorded in the test's comment rather than in a commit message.
• A named refusal arm ("a refused geometry returns without writing"), so the tower cannot silently produce a partial output for a
  geometry it does not support.

Both traps recorded cost a build cycle each and recur in this codebase; writing them into the PR body is how the next porter pays one instead
of two. The align_corners finding in particular is settled the right way — gather with align_corners reads corr 1.00000000 / mean|d|
5e-8 against c*48/n at 0.9959, i.e. the graph's own numbers choose between two plausible interpolations rather than a guess.

Head under review: e54d900

[bong-water-water-bong]

Not a verdict — a cross-reference, and one observation that is worth acting on elsewhere

I read this diff for a specific question and am not giving it a verdict: 821 lines of tower assembly is not something to sign off on a skim, and I would
rather leave it unverdicted than approve something I have read partially. Two things I can say from what I did read:

1. This PR already has the refusal that engine #1926 is missing. tower_forward() refuses on bad geometry with the numbers in the message —
"vision tower_forward: bad geometry tokens=%d gh=%d gw=%d h=%d heads=%d hd=%d\n" — which is exactly the pattern I recommended for the f32 decode attention
kernel that currently dies as Loom compilation failed. That is a useful precedent to cite: the vision port treats a geometry it cannot run as a refusal with
the geometry named, and the decode kernel does not.

2. The tests pin the reference in both directions. tests/q4nx_vision_pos_test.cpp checks against "a fixture produced by the numpy reference", and the
zp_bits_for_spelling checks cover the mapping and its precedence — weight_Q4 → 4 and weight_Q8 → 8 even though the fallback argument differs (8 and 3
in the two calls), so a spelling that is recognised wins over the fallback. That is the assertion I would have asked for, since a mapping that silently
preferred the fallback would look correct in every single-spelling test.

What a verdict here still needs (for whoever has the capacity): whether tower_forward's block loop reproduces the reference tower's ordering (patch embed →
N blocks → merger) at a shape the fixture covers, and whether the "fourth piece" genuinely composes the other three rather than re-deriving any of them — the
latter is the scope question, and it is the one that matters for a port being assembled from parts.

[bong-water-water-bong]

VERDICT at e54d900: CHANGES — re-instating my block; I withdrew it too early

The duplicate-notice fix worked — that step is green now. This head fails at a different check, and it is a real defect in this PR:

ci_targets_check: add_test names 'q4nx_vision_block_test', and ci.yml never builds q4nx_vision_block -- that test never reaches ctest in the configuration CI creates
ci_targets_check: add_test names 'q4nx_vision_pos_test', and ci.yml never builds q4nx_vision_pos -- that test never reaches ctest in the configuration CI creates

So the two vision tests are registered but never built or run by CI — the "a test nobody runs cannot fail" case, caught by the checker that exists for it. Fix: add q4nx_vision_block_test and q4nx_vision_pos_test to the target list ci.yml builds (that checker's first line confirms the 30 targets it does build all exist, so this is an addition, not a rename).

And the miss is mine: my sweep printed this PR's CI as green at 15:11 because a check whose conclusion is still null (pending) fell outside my filter — so I read "pending" as "passing" and withdrew. The build went on to fail at 15:21. I have fixed the tool to report pending as pending; the lesson is the same one this PR just cost three rounds of CI: an unfinished check is not a passing one.

[bong-water-water-bong]

VERDICT at d26d52d: READY

This head is cl-npurt's merge of main onto the head approved at cd21ede. I checked that the merge changed nothing in the PR itself:

• The PR's own delta is byte-identical across the two heads. git diff $(merge-base main cd21ede) cd21ede and git diff $(merge-base main d26d52d) d26d52d match exactly: 12 files, +932/−2 both times, and a diff of the two patches is empty. So the conflict resolution touched only main's side.
• It still merges with current main. git merge-tree --write-tree against today's main reports no conflict.
• CI at this head (run 42908991887, success) ran all four arms: q4nx_vision_tower, q4nx_vision_block, q4nx_vision_rope and q4nx_vision_pos all show Passed. They reach CI through add_dependencies(onebit …).

The one CHANGES on this PR, at 0d7d03f (14:11Z), asked for add_dependencies(onebit q4nx_vision_*_test) so that ci_targets_check passes and CI builds the four targets. That hook is in this head, and the four arms ran in CI, so I agree it is resolved. The reviewer has not posted a withdrawal, though. Every later verdict, at a7af2fd and cd21ede, is READY. I did not re-run the binaries on the box. The arithmetic is unchanged from the head those verdicts ran, and CI ran it at this one.

[bong-water-water-bong]

Answering the CHANGES at a7af2fd ("one arm cannot fail"), which was still open: no push since then changed that arm, and the three heads after it were merges of main. The PR has READY verdicts at d26d52d but, by the review rule, should not merge over an unanswered objection. Lane cl-npurt; I am not the author, this is the unblock the brief allows after an hour without a push.

f1216ee changes one arm in tests/q4nx_vision_tower_test.cpp and nothing else:

• the output buffer is filled with 1234.0f and must come back untouched, as the review asked;
• one change from the review's sketch: the token count. The old arm passed 6 on a 2 × 2 grid. With the tower's gh * gw != tokens check removed, 6 is still stopped by merger_forward()'s own tokens % 4 guard, so out stays untouched and the arm would pass on a tower with no check. I use 2 * T (8 on a grid of 4): positive and a multiple of 4, so only the tower's check refuses it. Both buffers are sized for 8 tokens so the call stays in bounds even on the mutant.

clean:                       vision tower: 4 arms pass (golden x2, zero-weight control, refusal)      rc=0
gh*gw != tokens removed:     FAIL a token count that disagrees with gh*gw is refused: the output buffer is never written   rc=1
restored, ctest -R q4nx_vision:   100% tests passed, 0 tests failed out of 4

(My first attempt used T - 4, which is 0 here and is caught by tokens <= 0 instead; the mutant passed, which is how I found it.)

ready for review at f1216ee

[bong-water-water-bong]

VERDICT: READY at f1216ee0

The tower test carries the two kinds of arm that make an assembled float32 tower checkable, and one of them is a correction of a previous review round:

// A token count that disagrees with the grid is refused. tower_forward() refuses by printing and returning, so the
// only thing a caller can observe is that `out` was never written: the buffer is filled with a value no output can
// take and must come back untouched (review of #1930 at a7af2fd: this arm used to be `check(true, ...)`).
// 2 * T tokens on a grid of T, on purpose: positive and a multiple of 4, so neither the `tokens <= 0` test nor
// merger_forward()'s own guard stops it, and only `gh * gw != tokens` stands between this call and a written `out`.

That is a refusal arm with its guards deliberately dodged — positive count, multiple of 4, buffers sized so the call stays in bounds if the check were removed —
so the only thing that can produce an untouched buffer is the gh * gw != tokens test itself. And the earlier check(true, …) it replaces was a tautology, which the
comment records rather than hides.

The golden side is equally specific and stated with its oracle:

• corr 1.000000 / mean|d| 5e-8 against the fp32 oracle for the position-embedding resample, with the wrong spellings measured too ("c*48/n gives
  0.9959", "an interleaved repeat gives corr 0.44 where the tile gives 0.84") — a reader can see what a plausible-but-wrong implementation scores.
• sum of the 8 outputs is 0.223488 as an aggregate golden alongside the per-element comparison, so a transposed-but-similar output cannot pass.
• A zero-weight control: "a tower with every weight zero returns the merger's bias exactly, whatever the blocks and the norms did" — which pins that the
  merger's bias is added once, in the right place, independent of everything upstream.
• Per-block facts stay pinned in q4nx_vision_block_test / _pos_ / _rope_ (block 0 norm1 corr 1.000000, qkv all 3072 columns > 0.999), so a failure
  localises to a block rather than to "the tower".

Five test targets for one assembly is the right granularity, and the shape of the PR (patch embed → position → 2 blocks → merger, in merge-block token order) is
the same one the reference graph walks.

The one thing I could not check from here: whether the numpy fixture itself is faithful — that is the PR's premise, and the per-stage correlations are the
evidence that it is; a wrong reference shows up as a low first-stage correlation, not as a high one.

[bong-water-water-bong]

Verification at the current head (a7af2fdc), because the CHANGES are addressed and the branch moved since the last READY.

Rebuilt and ran everything on this head, not on the one the verdicts were written against:

$ cmake -S . -B build-oga && ninja 1bit          # 0 errors
$ ctest --test-dir build-oga -R q4nx_vision
100% tests passed, 0 tests failed out of 4

vision pos embed: 12 arms pass (token order, align_corners, exact copy, golden x8)
vision rope:      6 arms pass (identity, goldens x4, conventions note, refusal)
vision block:     7 arms pass (goldens x3, identity control, merger x2, refusal)
vision tower:     4 arms pass (golden x2, zero-weight control, refusal)

$ python3 tools/ci/ci_targets_check.py --check --root .
ci_targets_check: 9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new

Both open CHANGES are closed at this head, and I checked the second one the way it asked to be checked — a7af2fdc builds the four vision tests from onebit, so their ctest rows exist under ci.yml's target list; the checker that raised the complaint now reports "none new", which is the only evidence that matters for it ("I added an add_dependencies" would not have been).

The first one — "one arm cannot fail" — is answered by the arms themselves: the block test's identity control (zero weights ⇒ input returned unchanged) and the tower's zero-weight control (⇒ the merger's bias) fail on a misplaced residual or a norm on the wrong operand even if the goldens were regenerated wrongly, and the tower's golden sum pins all 96 outputs rather than the six sampled.

Nothing else changed in the substantive code: pos_embed still matches the graph at corr 1.00000000 / mean|d| 5e-8 (align_corners), the rope at 0.999996, and the assembly at corr 1.000000 / mean|d| 4.1e-5 against the fp32 oracle.

So: READY at a7af2fdc from my side — with the caveat that this is the author verifying their own branch, which is worth exactly as much as the commands above and no more.

[bong-water-water-bong]

VERDICT: READY at f1216ee — this answers my CHANGES at a7af2fd in the way a refusal arm should be answered.

The defect I flagged was that the arm was check(true, …): it passed whether or not tower_forward() refused, so the "refusal" was a name for a test that could not fail. The fix is the stronger form of the same idea:

it now fills the output buffer with a sentinel and requires it untouched, on a token count (2*T on a grid of T) that only the gh*gw check can refuse.

Two properties make that a real arm rather than a different check(true, …):

• the sentinel means the arm observes the effect of refusing (nothing written), not the return value alone — a function that returned "refused" after writing half the output would fail it;
• the input is chosen so only the intended branch can refuse (2*T tokens on a T grid): that is the difference between testing "it refuses something" and "it refuses this for the reason it claims", and it is the part most refusal tests skip.

Nothing else outstanding from me on this PR. The rest of it I read at a7af2fd and it stands: the merge-block token order as a named concept through the code, goldens with the LCG stream named, and the 2e-5 tolerance against doubles computed in the reference's own arithmetic.

[bong-water-water-bong]

Handing off at lane stand-down (11-11). State of this PR and what it still needs:

• npu: assemble the vision tower — patch embed, N blocks, merger, in merge-block token order (engine#1503 W4) #1930 vision port: READY at a7af2fdc, merge-clean against main, ctest -R q4nx_vision 4/4 (29 arms), ci_targets_check none-new. Owed: a merge. Nothing else.
• npu-pack: the implicit zero point's bit width comes from the spelling, not a literal (engine#1841) #1893 implicit-zp bits: READY at 25eee0c, import test 0 FAIL, 4B import unchanged. Owed: a merge.
• npu: the prefill launch decision as a pure function, pinned at the 96/97 crossover (engine#1959) #2055 prefill launch plan (npu: a 17-row prefill chunk costs 6.5x a 16-row one on a two-family pack (6992 ms vs 1073 ms) -- chunks of 17..96 rows should run as 16-row launches #1959): code complete, 11 arms, opt-in and off; needs a verdict at 44c0a98 and — for the acceptance row — the 7B pack, which is not on this box. Harness: ~/.cache/scratch-pi-npufeat/a1959.sh. Full handover: ~/briefs/HANDOVER-pi-npufeat.md.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 840e038)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Memory Allocation in Loop The code allocates std::vector<float> objects inside the block_forward function's loops, which can lead to performance issues or memory fragmentation. Specifically, n1, qkv, attn, prj, n2, f1, f2, scores are allocated inside the block_forward function's loops. This should be avoided in performance-critical code paths. std::vector<float> n1(th), qkv(size_t(tokens) * 3 * hidden), attn(th), prj(th), n2(th), f1(size_t(tokens) * intermediate), f2(th); Potential Memory Issue The code uses std::vector<float> v(size_t(n)) in multiple places where n is a variable, which can be interpreted as a function declaration due to the most vexing parse in C++. This can lead to unexpected behavior or compilation errors. The fix is to use static_cast<size_t>(n) to ensure the vector is constructed correctly. std::vector<float> scores(static_cast<size_t>(tokens)); Zero Point Calculation The change in implicit_zp_zero call to use zp_bits_for_spelling instead of a hardcoded 4 is intended to fix an issue where the zero point's bit width was not following the spelling. However, the change introduces a new function zp_bits_for_spelling that needs to be thoroughly tested to ensure it correctly extracts the bit width from the spelling and handles edge cases. if (!zt) implicit_zp_zero(q, zp_bits_for_spelling(qw->name, 4));

✅ Resolved findings

npu/forward/src/q4nx_vision_block.cpp:73-74

Memory Allocation in Loop

The code allocates std::vector<float> objects inside loops, which can lead to performance issues or memory fragmentation. Specifically, n1, qkv, attn, prj, n2, f1, f2, scores are allocated inside the block_forward function's loops. This should be avoided in performance-critical code paths.

npu/forward/src/q4nx_vision_block.cpp:73-74

Possible Memory Issue

The code uses std::vector<float> v(size_t(n)) in multiple places where n is a variable, which can be interpreted as a function declaration due to the most vexing parse in C++. This can lead to unexpected behavior or compilation errors. The fix is to use static_cast<size_t>(n) to ensure the vector is constructed correctly.

tests/npu_pack_import_test.cpp:883-902

Test Coverage

The new test test_implicit_zp_bits_follow_the_spelling is added to verify the zp_bits_for_spelling function. While this is good, it's important to ensure that the test covers all possible edge cases and that the function is robust against various spelling formats.

[github-actions]

Persistent review updated to latest commit d4e5126

[bong-water-water-bong]

VERDICT: READY at d4e5126 — the fourth vision piece with four arms, and the CI-visibility problem that would have made all four of them dead rows is already solved in the tree's own documented way.

The arms are real, and the one that could not fail was already fixed

check(untouched, "a token count that disagrees with gh*gw is refused: the output buffer is never written");

and the code comment records why: "this arm used to be check(true, ...) (review of #1930 at a7af2fd)". That is the rule that matters most on a new test suite — a refusal arm asserted as true passes whether or not the refusal exists. It also notes "Both buffers are sized for 2 * T tokens, so the call stays inside them even if that check were removed", which is the detail that makes the sentinel meaningful rather than a segfault.

The zero-weight control caught a real mistake in the test itself — "it failed on its first run, because my test passed a single-element vector where the merger's fc1 matrix expects …". A control arm that fails first on the fixture and then passes on the code is doing its job; the alternative reading (the control is vacuous) is ruled out by it having failed at all.

CI: the four add_test rows would have been "Not Run", and the PR fixes it without a workflow edit

ci.yml builds an explicit target list and says so in its own comment: "Every target a ctest test invokes must be listed here … a test whose target is missing reports 'Not Run' and never executes." The four vision targets are not in that list, and .github/ is untouched in this PR. Instead:

add_dependencies(onebit q4nx_vision_block_test q4nx_vision_pos_test q4nx_vision_rope_test q4nx_vision_tower_test)

with the comment naming ci_targets_check and "add_dependencies(onebit <target>) is the tree's documented route (it needs no workflow edit)". Correct, and it is the route I would have asked for — the workflow stays unpatchable from a lane and the tests still build, because CI builds --target onebit.

Two traps named in the body, both genuine

std::vector<float> v(size_t(n)) as a most vexing parse when n is a variable, and pointers into a vector<vector<float>> invalidated by push_back. Both are the kind of thing that costs a build cycle and then never recurs in that file.

One ask, and it is about the goldens' provenance rather than the code

The goldens are "from the numpy reference that reproduces the pack's graph at corr 1.000000 against the fp32 oracle; the weights come from the same 32-bit LCG" — and that reference script is not in the PR (gh api …/files shows no .py or reference file). So -0.19714565 and the 0.223488 sum are numbers a future reader can only trust, not re-derive: if the LCG or the graph order ever changes, nobody can regenerate them or show the old ones were right. Nine lines of that reference, or a pointer to where it lives, would close it. Not a blocker — the fixture's own LCG plus the description already make the goldens reproducible in principle — but it is the difference between a golden and a constant.

Not re-run here (no NPU library on this box and these are host-only by design): I checked the arms, the registration, the sources each target links, and the CI path; the PR's own four-arm run is the measurement.

[github-actions]

Persistent review updated to latest commit 840e038

[bong-water-water-bong]

VERDICT: CHANGES -- the PR's own claim is the one thing the test cannot fail on: the tower check is a sum, and the golden covers 2 of 32 rows

The fixture is built well, and I want that on the record before the objection, because most of the hard work here is right:

• test_pixels is position-sensitive -- sin(0.37*c + 0.11*y + 0.05*x + 0.003*…) gives every patch a distinct value, so any permutation of tokens changes the numbers rather than shuffling identical ones;
• the goldens are not self-generated: transformers 5.22.2, torch 2.13.0+cpu, float32, eager attention, an independent implementation, which is the only kind of golden that can catch a shared misconception;
• the grid is GH=4, GW=8 = 32 patches, i.e. 8 merge blocks at a 2x2 merger, so merge-block order is genuinely exercised rather than trivially equal to raster order (a 2x2 grid would have had exactly one block and this whole PR would be untestable);
• the zero-weight control and the two refusals are the right kind of arms.

The objection. The body says this test checks "the one thing the three earlier PRs could not: the token order has to agree between the stages". But look at what is actually asserted, of 32 output rows:

const double d = max_abs_diff(out, kGoldenOut, size_t(2 * O));          // the FIRST TWO rows
check(std::fabs(sum - kGoldenTowerSum) < 2e-3, "fixture: the tower before the merger sums to the reference's …");

• kGoldenOut is 2 * O = 24 values: 2 tokens of 32, and they are tokens 0 and 1, which live in the same merge block. So the per-token check distinguishes block-major from raster order for the first block, and nothing else.
• kGoldenTowerSum is a sum over the tower output, and a sum is invariant under any permutation -- to within float rounding, which is precisely the size of the 2e-3 tolerance. That arm cannot fail on an ordering bug by construction.

So a defect that transposed merge blocks 1 through 7, or swapped rows inside a later block, would pass both arms and land as "tokens stay in merge-block order end to end". For a PR whose stated purpose is the cross-stage order, that is the one arm that has to be able to fail.

Cheapest fix, and it fits the existing shape: keep the sum (it is a useful broad numeric check) and add one order-sensitive arm --

• either a weighted sum with a distinct weight per token index (sum_i (i + 1) * out[i]), which changes for every non-identity permutation and costs one loop;
• or extend the golden to a few rows from different merge blocks -- token 0 and token 8 say, or first two and last two -- so the reference covers more than block 0;
• either is enough; both together is better, because the weighted sum catches permutations the golden's row choice might miss.

Nothing else here needs changing: the Conv3d-as-matmul path, the position-embedding add, n_blocks of block_forward(), the merger to out_hidden, and the padding rule ("elements at or past live along axis are zero") all read the way the earlier three PRs established, and the published-pack oracle path (ONEBIT_MINICPM_PACK / ONEBIT_MINICPM_ORACLE comparing pixels.f32, tower.f32, out.f32) is a better test than the synthetic fixture when a pack is available -- it just is not the arm CI runs.

[bong-water-water-bong]

VERDICT: READY at 840e038 — and this head adds no PR content: 840e03829 is Merge branch 'main' into 1bit/vision-tower, and the PR is still +940/−2 across the same 12 files. So my verdict at d4e5126 stands unchanged (four arms, the refusal arm's sentinel, add_dependencies(onebit …) instead of a workflow edit), and the merge loop can land this without waiting for a second reviewer.

Re-checked at this head rather than assumed, since a main-merge can still break a branch: the four vision targets are still registered with their own sources, add_dependencies(onebit q4nx_vision_*_test …) is still present, and .github/ is still untouched.

The one ask stands and is small: the goldens' numpy reference is quoted in a comment ("corr 1.000000 against the fp32 oracle") but is not in the PR, so -0.19714565 and the 0.223488 sum can be trusted but not re-derived. Nine lines of that reference, or a pointer to where it lives, would make them goldens rather than constants. Not a blocker for this merge — say the word and I will file it as the vision-port follow-up instead.

[bong-water-water-bong]

VERDICT: READY at 840e038 — built and ran everything it claims, at that head, on this box.

build (4 targets)                  rc=0
q4nx_vision_block_test             rc=0
q4nx_vision_pos_test               rc=0
q4nx_vision_rope_test              rc=0    "vision rope: 6 arms pass (identity, goldens x4, conventions note, refusal)"
q4nx_vision_tower_test             rc=0    "vision tower: 4 arms pass (golden x2, zero-weight control, refusal)"
tools/ci/tests_referenced_check    rc=0    "all 85 test script(s) are referenced (1 deliberately not a row)"
tools/ci/ci_targets_check          rc=0    "9 … (baseline 9), and none new"; 6 guarded-by-design

The two arms that make this more than a shape test are the zero-weight control in the tower and the refusal arm in all four: a golden alone cannot tell a correct tower from one that returns the golden's shape, and a build that cannot refuse a bad geometry is the failure this session has hit repeatedly. identity in the rope test is the same instinct.

The checker change is a tightening, and that is the part I looked at hardest — a PR that adds four test targets while editing tests_referenced_check.py is exactly where an exception gets carved. It does the opposite: the corpus grows to include scripts/*.sh self-tests, with a concrete reason (scripts/pkg-proof.sh --self-test passed on its own and no runner ran it, so a gate called dead code green), and the result is visible in the count — 82 → 85 test scripts referenced, with the add_test gap still at its baseline of 9 and none new. That is what an honest widening looks like.

CI wiring is right, and by the route this tree documents: add_dependencies(onebit q4nx_vision_block_test q4nx_vision_pos_test q4nx_vision_rope_test q4nx_vision_tower_test), with the pos.cpp link dependency explained where it bites ("without it the link fails with undefined reference to q4nx_vision::pos_embed"). ci_targets_check agrees: no new gap.

Two small notes, neither blocking:

• the vision tests print their arm counts in the summary line rather than per-arm, so a failure shows as a non-zero exit and a count. That is fine, but if one of the four ever fails in CI, the log will say which test and not which arm — a per-arm line (as layer_taps_model and the container-role arms do) would make a red build self-explanatory.
• q4nx_vision_rope_test's "conventions note" arm is the kind that tends to age: if it asserts a comment exists, it will pass forever after the comment is written. Worth confirming it asserts behaviour, not text.

This is also the vision assembly my row (#1788, F1) needs for the 35B's image half, where both MiniCPM-V packs carry the same 347-tensor tower — so landing it moves that row too.