[michakraus]

Summary

Part M5 of the GeometricMachineLearning 0.9 simplification: the layers and architectures share one method where a pair differed in one choice, four bugs are fixed, the multi-head attention builds its head keys once, and PositionalEncoding allocates on the backend of its input. The outputs do not change.

This pull request is the one M5. It combines the three builds of the part, and #349 and #350 are closed in its favour. The basis is this branch, because its MultiHeadAttention maps over 1:n_heads with _head_keys(Val(N)) and infers at every head count; #349 and #350 infer Any from 32 heads, which is the default of StandardTransformerIntegrator(32). From #350 come the ClassificationTransformer(dl) delegation and its keyword test. From #349 comes the Int conversion of the dimension in the SympNet layer constructors.

• Q/P pairs. One function, _shift(Val(C), z, f), adds f of one half of a (q, p) input to the other half: src/layers/sympnets.jl has 3 forward methods on a (q, p) input where it had 6, symplectic_attention.jl 2 where it had 4, linear_symplectic_attention.jl 1 where it had 2. Each of the three SympNet forward methods keeps the layer's dimension check, and checks the rows of p as well as those of q. The six SympNet layer constructors (GradientLayerQ/P, ActivationLayerQ/P, LinearLayerQ/P) convert the dimension to Int, so ActivationLayerQ(Int32(4), tanh) === ActivationLayerQ(4, tanh). GradientLayer.second_dim is an Int, and the aliases LinearLayerQ/LinearLayerP no longer name an unused parameter.
• Bool ladders. Each Bool keyword of VolumePreservingAttention, the two volume-preserving feedforward layers, SymplecticAttentionQ/P, SymplecticAutoencoder and ResNetLayer picks a Symbol type parameter in one expression. ResNetLayer encodes its bias as :bias/:no_bias, as the volume-preserving layers do; its use_bias::Bool keyword stays.
• Volume-preserving feedforward. VolumePreservingLowerLayer and VolumePreservingUpperLayer are aliases of one struct VolumePreservingFeedForwardLayer{M, N, bias, AT, Triangle}, with one initialparameters; the bias keyword is init_bias in every layer.
• MultiHeadAttention. One initialparameters for both weight kinds. The forward pass finds each head by its name head_i, from a key tuple that a generated function builds once for each number of heads, where it built Symbol("head_", i) on every pass.
• Architectures. LASympNet has one Chain method where it had four; one _gradient_layers builds the gradient layers of GSympNet, LinearSymplecticTransformer and SymplecticTransformer and replaces the two _make_block_for_initialization methods (known issue C19 leaves KNOWN_ISSUES.md); the DataLoader constructors of LASympNet and GSympNet, and ClassificationTransformer(dl) (src/architectures/transformer_neural_network.jl), delegate; GSympNet names its activation field activation; one encoder and one decoder serve every AutoEncoder that is not a SymplecticAutoencoder.
• Bugs. LinearSymplecticTransformer keeps its activation (it built new{typeof(tanh)}, so activation = sin raised a MethodError). StandardTransformerIntegrator's field upsacling_activation is spelled upscaling_activation. decoder of an AutoEncoder gives the decoder the decoder's block count, where it gave it n_encoder_blocks; the model and the parameters already read the right count, so no output changes. A SympNet layer raises its own Dimension mismatch. error for a p of the wrong size, where it failed later in the product or the broadcast.
• ::Type{T} everywhere under src/layers/.
• PositionalEncoding fills similar(x, eltype(x), dim, seq_length) by one broadcast, so it runs on a JLArray with scalar indexing disallowed; the "CPU only" docstring warnings go (known issues B11 and K5 leave KNOWN_ISSUES.md).

Breaking changes are listed in CHANGELOG.md: ResNetLayer's third type parameter, the one volume-preserving feedforward struct, init_bias! → init_bias, GSympNet.act → activation, and the upscaling_activation spelling.

Tests

• test/layers/layer_pins.jl and test/architectures/architecture_pins.jl: 27 layer cases, 13 architecture cases and the encoders and decoders of two autoencoders, each in Float32 and Float64, against the outputs that 4c4a8320 gives for the same seeds and inputs. The cases cover dense, SymmetricMatrix, triangular, SkewSymMatrix and Stiefel leaves. architecture_pins.jl also checks that the encoder and the decoder of an autoencoder with different block counts each keep their own count. layer_pins.jl has a self-test of the comparison: it shifts the entry -0.25 of [1.5, -0.25, 3] by 16 and by 17 eps(3); 16 passes and 17 fails, so the scale is the vector's and not the entry's, and a size mismatch gives Inf.
• test/layers/layer_structure.jl: the merged methods, the type parameters, the two bugs, the dimension check of q and of p, the Int dimensions of all six SympNet constructors, the MHA draw order and head lookup by name, and the ::Type{T} grep. The draw-order test checks that each Stiefel leaf is a StiefelManifold, drawn head by head as StiefelLayer draws them. The alias test reads the source line, because Julia drops an unused alias type variable.
• test/architectures/architecture_structure.jl: the merged architecture methods, and the ClassificationTransformer(dl) keyword test (from GML part M5: one functor per layer family, the Bool keywords as type parameters, and the MHA head keys built once #350) in both precisions.
• test/layers/multi_head_attention_allocation.jl: the forward pass allocates what the same head products allocate with constant keys, within 1024 B over 1000 passes, for one and two heads.
• test/layers/multi_head_attention_inference.jl: both forward methods infer at 4 and at 32 heads.
• test/architectures/forward_inference.jl: @inferred on the forward pass of every exported architecture with a chain, in Float32 and Float64, including StandardTransformerIntegrator(32) (32 heads by default).
• test/layers/positional_encoding.jl: the layer on a JLArray with scalar indexing disallowed.

The MultiHeadAttention draw order is unchanged, and no seeded pin moved. The one initialparameters draws head by head, and in each head the query, the key and the value projection, as before.

Out of the Bool-ladder item: MultiHeadAttention (Stiefel, add_connection) and ClassificationLayer (average) encode other choices and keep their Bool parameters, and the ANN Dense layer is outside this part.

The pinned outputs were recorded on arm64 and are compared at 16 ULPs of the largest magnitude of the pinned vector (PIN_ULPS in test/helpers/pins.jl). A comparison at 8 ULPs of each entry's own magnitude failed Julia 1 on Ubuntu and Windows at the full SymplecticAutoencoder Float64 pass (run 39577642351). That pass is the deepest case, encoder then decoder, and its output holds entries such as -0.0686 beside 1.276 that come out of cancellation. The rounding error of such an entry is set by the terms it came from, so a bound relative to the entry's own magnitude asks for an accuracy that floating point does not give. At 16 ULPs of 1.276 the bound is about 3.6e-15 in Float64; a wrong layer moves an output by a fraction, thousands of ULPs away. A failure prints the measured deviation: @test pin_ulps(vec(y), pin) ≤ PIN_ULPS gives Evaluated: <n> ≤ 16. The pins, the seeds, the inputs and the Stiefel fix are unchanged.

Suite at the critic-passed code: full suite with Metal on Julia 1.13.2, pass (Metal 40/40, one known broken: the JET @test_broken of #325); full suite with Metal on Julia 1.12.7 (the floor), pass (Metal 40/40, one broken); doctests pass; the Documenter docs build gives 0 unresolved references on Julia 1.13.2.

Critic

Verdict: PASS (round verify 2, branch simplify/part-m5-3 at 179d093, judged 7ada06d..HEAD). Mutants: 12 CAUGHT, 0 SURVIVED, 0 UNKNOWN, 0 INVALID. Blocking defects: none.

clause	verdict
the SympNet layers keep the parent's dimension check (round-1 reproducers 16/16 and 4/4 pass; probe 12/12; mutants G1–G4 CAUGHT)	PASS
MHA finds each head by its name, keys built once (reproducers 2/2 and 2/2; probe 66/66 in Float32 and Float64; mutants N3m, N3t, H1, H2, H3 CAUGHT)	PASS
check 4 pins and the LASympNet Chain signature (units pass; mutants N4, D1, D2 CAUGHT)	PASS
Zygote gradients through MHA unchanged (12 identical hashes against origin/main)	PASS
the forward pass of every architecture infers (forward_inference.jl)	PASS
the 18 test files that the fix diff reaches, and quality: 734 pass, 1 broken (the existing @test_broken isempty(reports) # #325 at quality/jet.jl:96)	PASS
CHANGELOG matches the code; KNOWN_ISSUES K20 and K21 evidence true	PASS

Earlier blocking defects (round 1): the SympNet dimension guard deleted; the MHA forward pass reading heads by field position. Both are fixed, and their reproducers pass.

The critic's verdict covers 179d093. The commits after it are not covered by it. 56c3443, 9ff44a5 and c03002a change text only: KNOWN_ISSUES.md, the headline of one CHANGELOG.md entry, and the header comment of test/layers/layer_structure.jl. e152ae7 merges origin/main. 52a01a8 also changes code: the Int conversion of the dimension in the SympNet layer constructors (src/layers/sympnets.jl), src/architectures/transformer_neural_network.jl, the pin tolerance from 8 to 16 ULPs (test/helpers/pins.jl), and several test files. The review fix commit after it changes src/architectures/autoencoder.jl and src/layers/sympnets.jl (known issues K23 and K24 below).

Checks of 52a01a8, which the critic did not judge:

• The six changed test files pass, 528/528, on Julia 1.13.2.
• Six mutants are CAUGHT: pin_ulps with each entry's own eps (the old scale); PIN_ULPS = 8; < for ≤ at the 16-ULP case (each by the comparison self-test); ActivationLayerQ keeping an Int32 dimension ("a SympNet layer stores its dimensions as Int"); the Stiefel branch of _head_weight drawing a plain matrix ("MultiHeadAttention draws Stiefel weights head by head"); ClassificationTransformer(dl) swapping Stiefel and add_connection ("a DataLoader gives the ClassificationTransformer of its dimensions").
• CI on 52a01a8 (run 40078895180): Julia 1 green on Ubuntu, Windows and macOS, so the 16-ULP bound holds on x86; Julia min green on Ubuntu and macOS; Doctests green; Julia min on Windows cancelled 10 s after the push of the next commit; Downgrade, pre and nightly red (red on main on 2026-10-21; not triaged against main).
• No local full suite ran on 52a01a8, because pre-push skips it on a topic branch.

Known issues added

IDs as in KNOWN_ISSUES.md at the head of this branch.

• K21 — LagrangianNeuralNetwork stores its activation in the field act. Present on 4c4a8320.
• K22 — a comment in src/layers/symplectic_attention.jl names a test file under test/parameters/, which does not exist. Present on 4c4a8320.
• K25 — the docstrings of LinearSymplecticTransformer and SymplecticTransformer say that n_sympnet = 1 gives one GradientLayerQ and one GradientLayerP; with init_upper = true the chain has two GradientLayerQ. Present on 4c4a8320.
• K26 — the SymplecticAutoencoder docstring says that decoder_init_q = true makes the first layer of each decoder block a p-type layer; it is a GradientLayerQ. Present on 4c4a8320.
• K27 — the UnknownDecoder docstring names its third argument n_encoder_blocks and links to itself for an example. Present on 4c4a8320.

K23 (the SympNet forward methods checked the rows of q only) and K24 (decoder gave the decoder of an AutoEncoder the encoder's block count) were recorded on this branch and are fixed on it, so they leave KNOWN_ISSUES.md again. Both were present on 4c4a8320.

Known issues closed: B11, C19, K5.

Reported, not fixed

• VolumePreservingFeedForward (the architecture) keeps its if/else init_upper ladder.
• LagrangianNeuralNetwork.act (K21).
• The stale comment path in symplectic_attention.jl (K22).
• The transformer docstrings on n_sympnet = 1 (K25).
• The SymplecticAutoencoder and UnknownDecoder docstrings (K26, K27).

Advisor decisions

For review, verbatim from the plan of this part:

Decided (advisor, 2026-10-19, high): check 4's last sentence reads: "encoder/decoder of a
SymplecticAutoencoder keep NonLinearSymplecticEncoder/NonLinearSymplecticDecoder, and those of
a PSDArch (SymplecticCompression) keep UnknownSymplecticEncoder/UnknownSymplecticDecoder, as
on the parent commit 4c4a8320." The branch's code stays as it is. The test asserts all three
families over REAL_ELTYPES: the SymplecticAutoencoder pair, the PSDArch pair, and a test-local
struct PlainAE <: AutoEncoder that gives UnknownEncoder/UnknownDecoder.

Decided (advisor, 2026-10-19, medium): check 3's "equals" is abs(measured - literal) ≤ 1024 B,
with k = 1000 forward passes inside one function-barrier call and literal > 0 as the control. One
rebuilt key costs at least 240 B per pass, so the mutant exceeds the tolerance by more than 230×.

(The plan corrects that figure to the measured one: one rebuilt key costs 192 B a pass on Julia 1.13.2, 187× the 1024 B tolerance. The tolerance stays as decided.)

Decided (advisor, 2026-10-20, high): check 2's "σ === tanh" reads "σ === Dense(4, 6, tanh).σ",
because AbstractNeuralNetworks' Dense stores Activation(tanh) = TanhActivation() (ANN
src/activation.jl:29, src/layers/dense.jl:5-8), so no tree can satisfy the literal. The test
also builds the architecture with the default upscaling_activation and transformer_dim = 6, and
asserts Chain(arch).layers[1].σ === Dense(4, 6, identity).σ. Tests catch: Chain calling
Dense(arch.sys_dim, arch.transformer_dim) without the activation picks Dense's default tanh,
and the default-activation assertion fails. A constructor that drops the keyword stores identity,
and the tanh assertion fails. Nothing changes in AbstractNeuralNetworks.

Decided (advisor, 2026-10-20, high): the MHA item's "The RNG draw order changes; re-pin any
seeded test" and the edge "The RNG draw order" predict what the sketch in gml-simplify.md §4.4
would do. They do not require that change. The item requires one initialparameters and the head
keys built once. A build that keeps the parent's draw order (head by head; in each head, query,
key, value) meets the item. It re-pins no seeded test, and the pull request says that the draw
order is unchanged and that no seeded pin moved. Tests catch: draws reordered to "PQ for every
head, then PK, then PV" in the one initialparameters make the StandardTransformerIntegrator
pins (n_heads = 2, dense MHA leaves, test/architectures/forward_pins.jl) fail at 8 ULPs.

Decided (advisor, 2026-10-20, medium): the Bool-ladder item reaches the four ladders it
lists, and each other GML layer that encodes one of the same choices. In GML, only ResNetLayer
does, for the bias. "The same kind in every layer" means a Symbol type parameter in those layers.
ResNetLayer encodes its bias as :bias/:no_bias, as the VPFF layer does, and dispatches its
initialparameters, parameterlength and forward methods on that Symbol. Its use_bias::Bool
keyword stays. MultiHeadAttention (Stiefel, add_connection) and ClassificationLayer
(average) encode other choices, have no ladder, and keep their Bool parameters. ANN Dense
is outside M5, and the pull request names the three as out of the item. Tests catch: for
use_bias in (true, false),
typeof(ResNetLayer(3; use_bias).parameters[3]) === typeof(VolumePreservingLowerLayer(3; use_bias).parameters[3]),
and ResNetLayer(3) isa ResNetLayer{3, 3, :bias, typeof(identity)}. The mutant that passes
use_bias through as a Bool fails both. In Float32 and Float64,
parameterlength(ResNetLayer(3; use_bias = false)) == 9 and == 12 with the bias.
ResNetLayer(3, tanh; use_bias = false) gives x + tanh.(W * x), with eltype === T. The
mutant that sends :no_bias to the bias method fails both.

(The advisor's test/architectures/forward_pins.jl is test/architectures/architecture_pins.jl on this branch.)

Pre-PR verification

Two passes. Pass 1 ran over the full branch. Pass 2 ran over the text-only commits after the critic's PASS, and it checked every CHANGELOG line that the branch adds.

Fixed

• Pass 1: the CHANGELOG headline of the Symbol-parameter change said too much. It now names ResNetLayer's bias parameter, the single volume-preserving feedforward struct, and the two renamed fields and the renamed keyword.
• Pass 1: the header comment of test/layers/layer_structure.jl now names the layers whose Bool keywords become Symbol parameters.
• Pass 1: K24 and K25 recorded. Pass 2: K26 and K27 recorded. Each is the same on 4c4a8320.

Unresolved

• None. The testset-name nit of pass 2 is resolved in 52a01a8: the testset in test/layers/layer_structure.jl reads "each Bool keyword of a ladder becomes a Symbol type parameter".

Checked and clean

• Correctness: the full diff was read. No stale callers of the renamed or removed names in src, test, docs, scripts or ext. The four LASympNet combinations match the base. The CHANGELOG counts and the K21, K23–K26 evidence reproduce.
• Type piracy: Aqua test_piracies passes.
• Type stability: the changed forward methods infer concretely in Float32 and Float64 (82 combinations). initialparameters(::MultiHeadAttention) cannot infer on the base either, because its keys depend on the runtime n_heads.
• Allocations: fatou lint is clean. The shared _shift methods and PositionalEncoding allocate the same bytes as the base bodies, with equal outputs, in a warm session. The 384 000 B difference for a two-head MultiHeadAttention over 1000 passes at the base reproduces (Julia 1.13.2).
• Comments and history: every comment next to a hunk was read. No history in the added lines outside CHANGELOG.md.
• The test layout check passes.

Not checked

• Inference on the 1.12 floor in the verifier; the suite, with forward_inference.jl, passed on 1.12.7.
• Fresh-process allocation figures in the verifier.
• A GPU device run; PositionalEncoding on a device is covered by the JLArray testset only. The Metal group does not run PositionalEncoding, and a broadcast on a JLArray also reads a host matrix, so the JLArray testset asserts where the encoding is allocated, not that a device kernel compiles.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f4fe7b1f-e89d-4c3b-8d5d-dcb3fed8966f

Commits

Reviewing files that changed from the base of the PR and between c03002a and 52a01a8.

Files selected for processing: 11

• CHANGELOG.md
• KNOWN_ISSUES.md
• src/architectures/transformer_neural_network.jl
• src/layers/sympnets.jl
• test/architectures/architecture_pins.jl
• test/architectures/architecture_structure.jl
• test/architectures/forward_inference.jl
• test/helpers/pins.jl
• test/layers/layer_pins.jl
• test/layers/layer_structure.jl
• test/layers/multi_head_attention_inference.jl

Files skipped from review as they are similar to previous changes: 1

• KNOWN_ISSUES.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

• Bug Fixes

  • Positional encoding now works with device-backed arrays and preserves their backend.
  • Transformer architectures now retain their configured activation, and architecture and encoder/decoder outputs are corrected.
  • Dimension handling and transformer scaling-activation naming are corrected.

• Compatibility

  • Some public layer and architecture field names, type parameters, and constructor keywords have changed. See the changelog for breaking-change details.

• Documentation

  • Updated transformer documentation to reflect positional encoding behavior.

This PR consolidates layer dispatch and architecture builders, updates multi-head attention initialization, and makes positional encoding allocate on the input backend. It also adds pinned-output, structure, allocation, and inference tests, and updates the changelog and known-issue records.

Changes: Layer and architecture consolidation

Layer / File(s)	Summary
Shared layer types and dispatch src/layers/*, test/layers/layer_structure.jl	Layer constructors, parameter initialization, and Q/P forward methods use consolidated type parameters and shared dispatch. GradientLayer.second_dim is restricted to Int, and SympNet constructors convert dimensions to Int.
Backend encoding and attention paths src/layers/multi_head_attention.jl, src/layers/positional_encoding.jl, src/layers/transformer.jl, test/layers/*	Multi-head attention uses shared initialization helpers and compile-time head keys. Positional encoding creates its matrix on the input backend. Tests cover device-backed encoding, attention allocation, and inference with 32 heads.
Architecture construction and wrapper dispatch src/architectures/*, test/architectures/architecture_structure.jl, test/integration/documented_defaults.jl	Architecture builders use shared gradient-layer construction and unified encoder/decoder wrapper selection. The transformer integrator field is renamed to upscaling_activation; DataLoader constructors forward to dimension-based or positional constructors.
Pinned outputs and inference coverage test/helpers/*, test/layers/layer_pins.jl, test/architectures/architecture_pins.jl, test/architectures/forward_inference.jl, test/runtests.jl	Tests compare seeded layer, architecture, encoder, and decoder outputs with recorded pins. They also check output element types, inference, and expected LASympNet layer sequences.
Release and issue records CHANGELOG.md, KNOWN_ISSUES.md	The changelog records API and implementation changes. The known-issues file removes entries about positional encoding and duplicate architecture builders and adds entries for remaining issues.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Refactor · Severity of issue fixed: Low

Merge Risk: Minimal · up to 52a01

The decoder metadata mismatch remains a pre-existing issue. This change is mergeable on the reviewed evidence.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly summarizes the main changes: consolidating Q/P methods, encoding Boolean choices, and unifying triangular layers.
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The pull request description directly covers the layer and architecture simplifications, bug fixes, backend allocation changes, breaking changes, tests, validation results, and known limitations shown…

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (11 skipped: 11 unsupported.)

Full details: Description check

Explanation

The pull request description directly covers the layer and architecture simplifications, bug fixes, backend allocation changes, breaking changes, tests, validation results, and known limitations shown in the changeset.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 92ffaa16-27f6-49ca-9fd9-8942f33614e7

📥 Commits

Reviewing files that changed from the base of the PR and between 4c4a832 and c03002a.

📒 Files selected for processing: 29

• CHANGELOG.md
• KNOWN_ISSUES.md
• src/architectures/autoencoder.jl
• src/architectures/linear_symplectic_transformer.jl
• src/architectures/standard_transformer_integrator.jl
• src/architectures/symplectic_autoencoder.jl
• src/architectures/symplectic_transformer.jl
• src/architectures/sympnet.jl
• src/layers/linear_symplectic_attention.jl
• src/layers/multi_head_attention.jl
• src/layers/positional_encoding.jl
• src/layers/psd_like_layer.jl
• src/layers/resnet.jl
• src/layers/symplectic_attention.jl
• src/layers/sympnets.jl
• src/layers/transformer.jl
• src/layers/volume_preserving_attention.jl
• src/layers/volume_preserving_feedforward.jl
• test/architectures/architecture_pins.jl
• test/architectures/architecture_structure.jl
• test/architectures/forward_inference.jl
• test/helpers/pinned_outputs.jl
• test/helpers/pins.jl
• test/integration/documented_defaults.jl
• test/layers/layer_pins.jl
• test/layers/layer_structure.jl
• test/layers/multi_head_attention_allocation.jl
• test/layers/positional_encoding.jl
• test/runtests.jl

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[michakraus]

Review of 52a01a8

Verdict: request changes (posted as a comment review, because the author is the account owner).

Findings

#	severity	file:line	claim	evidence
1	bug (from CodeRabbit, accepted)	src/architectures/autoencoder.jl:236	decoder(::NeuralNetwork{<:AutoEncoder}) passes arch.n_encoder_blocks into the field n_decoder_blocks of UnknownDecoder / UnknownSymplecticDecoder. The PR rewrites this line and keeps the defect. Its reason, "kept so that the outputs stay unchanged", does not hold: nothing reads the field.	Fields: autoencoder.jl:113 and :125 are n_decoder_blocks. decoder_model reads arch.n_decoder_blocks through compute_decoder_iterations (:140), and decoder_parameters counts the layers of decoder_model (:172). A grep of src/ finds no read of n_decoder_blocks on a Decoder. Probe on the head: a custom AutoEncoder with (6, 2, 2, 3) gives UnknownDecoder(6, 2, 2) with 2 decoder layers, which is what 3 decoder blocks build. With the method redefined to pass arch.n_decoder_blocks, the four decoder pin outputs are identical before and after, at 0.0 ULPs from AUTOENCODER_PINS.
2	quality	src/layers/symplectic_attention.jl:131, src/layers/sympnets.jl:225-239, src/architectures/linear_symplectic_transformer.jl:22, src/architectures/symplectic_transformer.jl:35, src/architectures/symplectic_autoencoder.jl:36, src/architectures/autoencoder.jl:102,108	The PR records five existing defects as known issues (K22, K23, K25, K26, K27) instead of fixing them. Each one is in code that the diff rewrites, or is the docstring of that code. Each fix changes one comment, one docstring line, or one check, and moves no output. K23's own entry says that its fix changes only the message of an error that is raised already.	KNOWN_ISSUES.md K22-K27 at the head, and the diff hunks at those locations: the comment at :131 sits directly above the rewritten symmetric forward method; the three rewritten SympNet forward methods each keep the size(x.q, 1) check alone; the Chain builders of both transformers and the SymplecticAutoencoder constructor are rewritten; UnknownDecoder is the type that finding 1's line builds.
3	quality	PR body	The PR body does not describe the head. (a) It says that the pins compare at 8 ULPs, but test/helpers/pins.jl:24 is const PIN_ULPS = 16. (b) It numbers the new known issues K20-K26, but the file has K21-K27, and K20 is the existing DataLoader((q = ..., p = ...)) entry. (c) It says that "the two commits after the critic's PASS change text only", but 52a01a8 changes src/layers/sympnets.jl (the Int conversions) and src/architectures/transformer_neural_network.jl (the ClassificationTransformer(dl) delegation). The critic verdict on 179d093 therefore does not cover the head. (d) It lists the Bool-ladder testset name as unresolved, but 52a01a8 renames that testset.	git show --stat 52a01a88; grep -n '^### ' KNOWN_ISSUES.md; pins.jl:24.
4	quality	KNOWN_ISSUES.md (B11 removal), test/devices/metal.jl	B11 is closed on a JLArray proxy. The proxy cannot show B11's failure: the test's own comment says that a broadcast on a JLArray also reads a host matrix, so it checks the type of the encoding instead. The metal group does not run PositionalEncoding, so no real device runs the layer whose GPU failure B11 recorded. One forward pass in test/devices/metal.jl closes this.	grep -rn PositionalEncoding test/devices/ gives no hit. The testset at test/layers/positional_encoding.jl:165-184 runs on JLArray only; see its comment at :173-174.
5	nit	KNOWN_ISSUES.md K23 location	The location src/layers/sympnets.jl:224 is a blank line at the head. The check is at :226. The two comment lines that 52a01a8 adds at :153-154 moved it. Name the function, not the line.	sed -n 224p src/layers/sympnets.jl prints an empty line.

CodeRabbit

• Inline comment on autoencoder.jl:236 (id 4335228541): accepted. This is finding 1. I checked each part of the claim at 52a01a8. The PR rewrites that line: true. The third argument fills n_decoder_blocks of both decoder types: true (:113, :125). decoder_model and decoder_parameters read n_decoder_blocks: true. No pin moves: true, because the decoder pin outputs are bit-identical with the fix applied. "KNOWN_ISSUES.md K23 records this defect": this was true at c03002a, which CodeRabbit reviewed. At the head, the entry is K24, and K23 is the SympNet half-size check. When the fix lands, K24 leaves the file, and K27 (the UnknownDecoder docstring) should leave with it. In practice, PSDArch always has 2 and 2 blocks and SymplecticAutoencoder has its own decoder, so only a user-defined AutoEncoder shows the wrong field.
• Summary comment: rejected where it states a fact. "Architecture and encoder/decoder outputs are corrected" is not true. The 88 pinned outputs (layers, architectures, encoders and decoders, in Float32 and Float64) hash identically on main (e759f20) and on the head. The summary has no other actionable claim.

Verified, and the result held

• Outputs do not change. I ran the PR's layer_pins.jl and architecture_pins.jl against main e759f20 and against the head, each in its own environment that develops that tree (Julia 1.13.2, arm64). Both pass 202/202. All 88 outputs hash identically on the two trees, so they are bit-identical here, not only within 16 ULPs.
• MHA key allocations, measured with --check-bounds=auto in one cold process per tree, minimum of 3 runs of 1000 passes: on main, the forward pass allocates 192 000 B (1 head) and 384 000 B (2 heads) more than the constant-key reference. On the head, the difference is 0 B for both. The CHANGELOG figure reproduces.
• Test files that the PR adds or reaches, run on the head through run-tests.jl (Julia 1.13.2): layer_structure, architecture_structure, positional_encoding, multi_head_attention_allocation, multi_head_attention_inference, forward_inference, symplectic_attention_network_parameters_gradient, resnet_tests, volume_preserving_feedforward, sympnet_layers_test, gradient_layer_tests, transformer_gradient, linear_symplectic_transformer, symplectic_transformer_chain, integration/documented_defaults, quality/aqua. 844/844 pass.
• _gradient_layers alternation is the same as the old is_upper_criterion for both init_upper values. The LST and ST builders repeat one tuple where the old code restarted the count in each block, so the layers are the same.
• RNG draw order in the merged initialparameters of ResNet, VPFF and MHA is weight before bias, and per head Q, K, V. This is the same as on main, and the bit-identical pins confirm it.
• Renamed names have no stale callers: no init_bias!, upsacling, _make_block_for_initialization or GSympNet .act in src, test, docs, scripts, ext or README.md. The docs/ @asserts on VolumePreservingLowerLayer{...} still hold through the 4-parameter alias.

CI, read from the jobs

• Downgrade - ubuntu-latest: known red, not caused by this PR. julia-downgrade-compat forcedeps fails before any test runs: GPUArraysCore 0.2.1 > 0.2.0, Zygote 0.7.5 > 0.7.0, ChainRulesCore 1.25.1 > 1.0.0, NNlib 0.9.30 > 0.8.0, KernelAbstractions 0.9.38 > 0.9.0. The same five errors occur on main e759f20 (run 39978614949). The PR does not touch Project.toml.
• Julia nightly - ubuntu-latest (1.15.0-DEV.111): known red, not caused by this PR. GPUCompiler fails to precompile with "UndefVarError: libLLVMExtra not defined in LLVM.API", Metal is skipped, and the job dies before any test. main fails the same way on 1.15.0-DEV.100.
• Julia pre - ubuntu-latest (1.14): the same GPUCompiler precompile failure, also on main. Known red, not caused by this PR.
• Doctests, Metal (Julia 1) and Metal (Julia min) pass. Julia 1 and Julia min on ubuntu, macOS and windows, Documentation and Scripts were still pending at the time of this review. These jobs are the only cross-platform check of the 16-ULP pin tolerance.

Not checked

• PositionalEncoding on Metal: my local Metal session stalled before it ran.
• Inference and the suite on the 1.12 floor: the pending Julia min jobs cover this.
• The need to widen the tolerance from 8 to 16 ULPs: on arm64 the outputs are bit-identical to main. The need is on other platforms, which the pending Linux and Windows jobs show.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.61%. Comparing base (e759f20) to head (3e9b7ad).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   87.10%   91.61%   +4.50%     
==========================================
  Files          58       58              
  Lines        1342     1228     -114     
==========================================
- Hits         1169     1125      -44     
+ Misses        173      103      -70     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[michakraus]

Review findings: resolution after 3e9b7ad

This resolves each finding of the review on 52a01a8 by its number. Commit 3e9b7ad is pushed to simplify/part-m5-3. The PR body is updated to match the head.

#	severity	finding	action	evidence
1	bug	decoder(::NeuralNetwork{<:AutoEncoder}) stores n_encoder_blocks in the field n_decoder_blocks (CodeRabbit, known issue K24)	fixed in 3e9b7ad	The method passes arch.n_decoder_blocks. New assertions in test/architectures/architecture_pins.jl ("encoder and decoder types of each autoencoder family") use an autoencoder with 2 encoder and 3 decoder blocks. On 52a01a8 the decoder is UnknownDecoder(6, 2, 2); after the fix the testset passes, 9/9 per element type. All architecture pins still pass, so no output moves. K24 leaves KNOWN_ISSUES.md.
2	quality	five existing defects recorded as known issues instead of fixed	K23 fixed in 3e9b7ad; K22, K25, K26, K27 not fixed	K23: the three SympNet forward methods on a (q, p) input check size(x.q, 1) == size(x.p, 1) == M ÷ 2. These lines are rewritten by the PR. Two new cases in "a SympNet layer raises on a half of the wrong size" (wrong p, matrix and vector) fail on 52a01a8 with 44 failures (DimensionMismatch from the matrix product), and pass after the fix, 118/118 per element type. layer_pins.jl, forward_inference.jl, sympnet_layers_test.jl, gradient_layer_tests.jl, sympnet_upscaling.jl, qp_dispatch.jl, psd_architecture_tests.jl, symplectic_autoencoder_tests.jl and sympnet_integrator.jl pass (Julia 1.13.2, arm64). K22, K25, K26 and K27 are a comment and three docstrings outside every hunk of this PR, so they stay recorded for a follow-up. K25 also has two possible fixes (correct the docstring, or change the chain, which changes the network), and that choice is the author's.
3	quality	the PR body does not describe the head	fixed (PR body edited)	The body now says 16 ULPs (PIN_ULPS in test/helpers/pins.jl), uses the IDs of KNOWN_ISSUES.md (K21, K22, K25–K27 open; K23, K24 fixed on the branch), states that the critic's verdict covers 179d093 and that 52a01a8 also changes src/layers/sympnets.jl, src/architectures/transformer_neural_network.jl, the pin tolerance and tests, and marks the testset-name nit as resolved in 52a01a8. The quoted critic table and advisor decisions are left verbatim.
4	quality	B11 is closed on a JLArray proxy only	not fixed	A fix needs a device run or a PositionalEncoding test in the Metal group (test/devices/metal.jl), a file this PR does not touch. No Metal session was available to this fix. The PR body's "Not checked" now states the limit of the JLArray testset. Open question for the author: keep B11 closed on this evidence, or keep it in KNOWN_ISSUES.md with kind not verified until a Metal run.
5	nit	K23's location cites a blank line	moot	K23 leaves KNOWN_ISSUES.md in 3e9b7ad.

CodeRabbit

• Inline comment on src/architectures/autoencoder.jl:236: accepted, and fixed as finding 1. One correction: at 52a01a8 the entry for this defect is K24, not K23; K23 is the SympNet half-size check.
• Summary comment, "architecture and encoder/decoder outputs are corrected": rejected. The PR changes no output. The review ran the 88 pinned outputs on main and on 52a01a8 and found one combined hash on both trees. After 3e9b7ad the layer and architecture pins still pass.
• The CodeRabbit review body lists files and run data only, and has no claim to resolve.

CI on 52a01a8

Downgrade - ubuntu-latest, Julia nightly - ubuntu-latest and Julia pre - ubuntu-latest failed. Each is known red and not from this PR: main at e759f20 fails the same way (the julia-downgrade-compat forced-dependency check, and the GPUCompiler precompile error libLLVMExtra not defined in LLVM.API). None of the three is a required check. The required checks are the Julia min and Julia 1 jobs on ubuntu, macOS and windows, and Doctests - ubuntu-latest. They run again on 3e9b7ad.

🤖 Generated with Claude Code