Repository navigation
GML part M5: one functor per layer family, the Bool keywords as type parameters, and the MHA head keys built once - #349
michakraus wants to merge 7 commits into
Conversation
… VPFF struct, and the MHA keys built once
Each SympNet layer family and each symplectic attention weighting had one functor per half of the input; each is one functor on _shift(Val(C), z, f) now, and the outputs are pinned bit for bit.
The Bool keywords of VolumePreservingAttention, the volume-preserving feedforward layers, SymplecticAttention and SymplecticAutoencoder are their type parameters, so no if/else ladder over new{...} remains. VolumePreservingLowerLayer and VolumePreservingUpperLayer are const aliases of one struct with a triangle parameter, and init_bias is the one bias-initialiser keyword.
MultiHeadAttention has one initialparameters for both Stiefel flags, and the head keys are built there rather than per forward pass; a forward pass allocates no more than 16 bytes above the same products with literal keys, in both element types, on a matrix and a 3-tensor, with one and two heads.
The architectures build their gradient layers through one _gradient_layers, LASympNet has one Chain, the DataLoader constructors delegate, GSympNet's field is activation, encoder and decoder dispatch on the architecture family, and the two _make_block_for_initialization methods are gone. LinearSymplecticTransformer keeps the activation it is given, and StandardTransformerIntegrator's upscaling Dense takes its keyword.
PositionalEncoding allocates its matrix on the input's backend, so it runs on a JLArray under allowscalar(false). Every exported architecture's forward pass is checked with @inferred in Float32 and Float64.
Closes known issues B11, C19 and K5, which leave KNOWN_ISSUES.md.
…he pins The consolidated SympNet functors had dropped the `size(z.q, 1) == M ÷ 2` check the six origin/main functors opened with, so a half of the wrong length reached the broadcast and, for an ActivationLayer, returned a value. The check is back at the top of all three functors, and the tests cover the NamedTuple, array and uneven-half paths in both precisions. The six layer constructors convert the dimension to an `Int`, so `GradientLayerQ(Int32(4), 8, tanh)` is the same type as `GradientLayerQ(4, 8, tanh)`. `LinearLayerP` loses the unused third alias parameter that the changelog already said was gone. `PositionalEncoding` builds its encoding on `KernelAbstractions.get_backend` rather than `networkbackend`, so a static array is accepted again, and the test asserts the backend of the array the layer adds. The test also restores the `allowscalar` setting it found. The forward-pass test lists `ClassificationTransformer` and covers a Stiefel and a `SkewSymMatrix` leaf; the allocation check measures 1000 passes against a 1024-byte tolerance; and the VPFF, LASympNet, extraction and shift pins carry the parent's numbers with element-type assertions in both precisions. The extraction test states all three autoencoder families over `REAL_ELTYPES`. Three false or stale CHANGELOG lines, four false or historical comments and the dead type assertion in the MHA tensor method are corrected. K20 records the VPFF docstring's formula, which omits the input term.
…nd pin the extraction on fixed leaves The JLArray testset of `positional_encoding` set scalar indexing with `allowscalar(false)`, which writes the process-wide default as well as the task-local key, and restored both from one captured value. It now runs under an outer `@allowscalar` and an inner `task_local_storage(:ScalarIndexing, ScalarDisallowed) do`, each of which puts back what it found; the test reads `requested_scalar_indexing[]` and never writes it, asserts that a scalar read raises inside the block, and asserts both states after it. The extraction pins of `gradient_layer_builders` overwrite every `StiefelManifold` leaf with a fixed, exactly orthonormal signed selection before the pinned pass, because a leaf is the CholeskyQR2 of a seeded draw and OpenBLAS 0.3.29 and 0.3.30 factorise the same draw to different last bits. The four vectors are recorded from the parent commit and are identical on Julia 1.12.7 and 1.13.2. The MHA value pins assert the element type and the shape of both outputs, the static-array testset runs in both precisions with `eltype(y) === T`, and the key-cost comment states the measured 192 bytes a pass.
…-order finding The family comment in `autoencoder.jl` said a `SymplecticCompression` always gets the symplectic `Unknown…` architecture, which a `SymplecticAutoencoder` does not. The header of `forward_pass_inference.jl` counted the volume-preserving transformer twice, `attention_shift.jl` referred to the plan, and the `Stiefel` note in the allocation test said the method branches on the flag. `KNOWN_ISSUES.md` gains K21, the order sensitivity of the allocation comparison in a process that ran the forward-inference coverage first, and its K20 entry names the two manual lines in the order they state their formulas.
`positional_encoding` documents its backend method, which the branch added on an exported function. Four test comments lose a word that recounted the change. The `MultiHeadAttention` draw-order entry moves to *Changed (breaking)*, because a seeded caller must re-pin, and the `upscaling_activation` rename keeps one entry, the breaking one.
The documentation entry names the `Transformer` docstring as well as `PositionalEncoding`, since the diff removes the CPU-only sentence from both. K20 moves to the Documentation group, and K22 records the pre-existing `decoder_init_q` sentence of the `SymplecticAutoencoder` docstring, which says `p` where the dispatch makes a `q` layer. K21 states the per-pass key figure as a figure and not as a comparison.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 86.72% 91.76% +5.04%
==========================================
Files 58 58
Lines 1341 1239 -102
==========================================
- Hits 1163 1137 -26
+ Misses 178 102 -76 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 Summary
Merge Risk: 🔵 Low · up to
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
f72e7edd-ed3c-40f2-b6e6-c7daa173f5fd
📒 Files selected for processing: 33
CHANGELOG.mdKNOWN_ISSUES.mddocs/src/architectures/volume_preserving_feedforward.mdsrc/architectures/autoencoder.jlsrc/architectures/linear_symplectic_transformer.jlsrc/architectures/standard_transformer_integrator.jlsrc/architectures/symplectic_autoencoder.jlsrc/architectures/symplectic_transformer.jlsrc/architectures/sympnet.jlsrc/layers/linear_symplectic_attention.jlsrc/layers/multi_head_attention.jlsrc/layers/positional_encoding.jlsrc/layers/psd_like_layer.jlsrc/layers/symplectic_attention.jlsrc/layers/sympnets.jlsrc/layers/transformer.jlsrc/layers/volume_preserving_attention.jlsrc/layers/volume_preserving_feedforward.jltest/architectures/forward_pass_inference.jltest/architectures/gradient_layer_builders.jltest/architectures/transformer_constructor_bugs.jltest/helpers/cayley.jltest/helpers/ulps.jltest/integration/documented_defaults.jltest/kernels/cayley.jltest/layers/attention_shift.jltest/layers/bool_type_parameters.jltest/layers/multi_head_attention_allocation.jltest/layers/positional_encoding.jltest/layers/sympnets_shift.jltest/layers/volume_preserving_attention.jltest/layers/volume_preserving_feedforward.jltest/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.
| NeuralNetwork( | ||
| UnknownDecoder(nn.architecture.full_dim, nn.architecture.reduced_dim, nn.architecture.n_encoder_blocks), | ||
| decoder_model(nn.architecture), | ||
| _unknown_decoder(arch)(arch.full_dim, arch.reduced_dim, arch.n_encoder_blocks), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use arch.n_decoder_blocks for the extracted decoder.
UnknownDecoder and UnknownSymplecticDecoder declare the third field as n_decoder_blocks. Line 239 fills that field from arch.n_encoder_blocks. The code then builds the decoder model from compute_decoder_iterations(arch), which uses n_decoder_blocks. If an AutoEncoder has n_encoder_blocks != n_decoder_blocks, the extracted decoder reports the wrong block count. The chain it wraps still has the correct layers. This line changed in this PR, so fix the field source now.
🐛 Proposed fix
--- "a/src/architectures/autoencoder.jl"
+++ "b/src/architectures/autoencoder.jl"
@@ -236,7 +236,7 @@
function decoder(nn::NeuralNetwork{<:AutoEncoder})
arch = nn.architecture
NeuralNetwork(
- _unknown_decoder(arch)(arch.full_dim, arch.reduced_dim, arch.n_encoder_blocks),
+ _unknown_decoder(arch)(arch.full_dim, arch.reduced_dim, arch.n_decoder_blocks),
decoder_model(arch),
decoder_parameters(nn),
networkbackend(nn))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _unknown_decoder(arch)(arch.full_dim, arch.reduced_dim, arch.n_encoder_blocks), | |
| _unknown_decoder(arch)(arch.full_dim, arch.reduced_dim, arch.n_decoder_blocks), |
- Pins compare at 16 ULPs of the pinned vector's largest magnitude, and a failure prints the measured deviation (pin_ulps), as decided for M5. - ClassificationTransformer(dl) calls the dimension constructor, as in #350. - ActivationLayerQ/P and LinearLayerQ/P convert the dimension to Int, as in #349, so an Int32 dimension names the same layer. - Tests: the Stiefel branch of the MHA initialisation and its draw order; inference at 32 heads (layer and StandardTransformerIntegrator(32)); the alias test reads the source, because Julia drops an unused alias variable; the stale header of the MHA inference test; a narrower name for the Bool-ladder testset. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Closed in favour of #351, which is now the one build of part M5. #351 takes this PR's rule that every SympNet layer constructor converts its dimension to The branch |
Part M5 — layers and architectures
Part M5 of the plan Simplify GeometricOptimizers and GeometricMachineLearning for 0.9: the layer
and architecture consolidations of GeometricMachineLearning, and three defects.
What changes
GradientLayer,ActivationLayerandLinearLayereach had aQfunctor and aPfunctor; each has one now, through_shift(Val(C), z, f).SymplecticAttentionandLinearSymplecticAttentionlose their per-direction functors the sameway. The outputs are unchanged bit for bit.
Boolkeywords areBooltype parameters:skew_syminVolumePreservingAttention,use_biasin the volume-preserving feedforward layers,symmetricinSymplecticAttention, andthe two init keywords of
SymplecticAutoencoder. A caller that names these types by theirparameters writes
trueorfalsewhere it wrote a symbol.VolumePreservingLowerLayerandVolumePreservingUpperLayerare aliases of one concreteVolumePreservingFeedForwardLayer{M, N, UseBias, AT, Triangle}. Both names stay exported, andthe bias initialiser has one spelling,
init_bias.MultiHeadAttentiongets oneinitialparametersmethod for both values ofStiefel, and itshead keys are built once rather than in every forward pass. The forward pass reduces over the
parameter
NamedTuples and names no key._gradient_layers;LASympNet'sfour
Chainmethods become one; theDataLoaderconstructors delegate to the dimension ones;GSympNet.actisactivation; andencoder/decoderdispatch on the architecture family ratherthan repeat themselves for
SymplecticCompression.LinearLayerQ/LinearLayerPlose an unused third parameter,GradientLayer.second_dimisInt, and five layers annotate their element type as::Type{T}with no new bound on
T. C19 leavesKNOWN_ISSUES.md.LinearSymplecticTransformer's inner constructor builtnew{typeof(tanh)}andconverted its
activationto it, soactivation = sinraised aMethodError;StandardTransformerIntegrator's fieldupsacling_activationis spelledupscaling_activation;and
PositionalEncodingallocates its encoding on the input's backend by a kernel, so the layerruns on a device array under
allowscalar(false). B11 and K5 leaveKNOWN_ISSUES.md.test/layers/sympnets_shift.jl,attention_shift.jl,bool_type_parameters.jl,multi_head_attention_allocation.jl,test/architectures/transformer_constructor_bugs.jl,gradient_layer_builders.jlandforward_pass_inference.jlare new. The shift tests pin thelayers' outputs bit for bit against an independent formula;
forward_pass_inference.jlruns@inferredon the forward pass of every exported architecture inFloat32andFloat64.The MHA draw order, and the seeded tests
MultiHeadAttentionnow draws every query projection, head by head, then every key, then everyvalue; it drew a query, a key and a value per head before.
No seeded pinned value in the suite changed, and no test needed re-pinning. No test pins a drawn
MultiHeadAttentionweight: the layer's tests assert types and shapes(
test/layers/transformer_setup.jl:12,test/layers/multi_head_attention_stiefel_setup.jl:22),orthonormality to a tolerance (
@test check(A) < tol), the optimizer-cache types, and the type ofthe pullback;
test/layers/transformer_optimizer.jl:34-35compares one network's parameters withthemselves before and after a step; and the new allocation test compares the layer's own output with
the same expression written out with literal head keys, which no draw order changes. The two full
suite runs — 1.13.2 and 1.12.7, both green — are what confirms it: a seeded pin on these weights
would have failed.
The CHANGELOG entry says the same, under Changed (breaking): "A seeded
MultiHeadAttentiontherefore draws different initial weights from the ones it drew before, so a caller that pins seeded
initial weights must re-pin them." That is a statement about callers outside this repository, not
about this suite.
Known issues added
VolumePreservingFeedForwardLayerdocstring's formula omits the input term(
src/layers/volume_preserving_feedforward.jl:7). Docs, not code: the functors and the manualpage both add the input.
MultiHeadAttentionallocation comparison is sensitive to what ran before it in thesame process (
test/layers/multi_head_attention_allocation.jl:67-83). It passes in the suite andas a named run; a supplemental reporter that replays the forward-inference coverage first reports
332240 B of difference for
Float32with one head. The tolerance is not widened.SymplecticAutoencoderdocstring saysdecoder_init_qmakes the first decoderlayer of
ptype, and it makes it aqlayer(
src/architectures/symplectic_autoencoder.jl:36). The sentence is wrong, not the dispatch; theline predates the branch.
K20, K21 and K22 are the entries this branch adds. It also removes B11, C19 and K5, whose fixes are
in it.
Critic
Verdict: PASS. Branch
simplify/part-m5at5b48e09. Judged0d00b98f..HEAD.scalar_restoration.jlscalar_states.jl, warm and fresh processstatic_precision_coverage.jl; unitmha_type_coverage.jl; unitkey_cost_comment.jl; key mutantsextraction_pins.jlon parent, 1.13.2 and 1.12.7reproducers.jlpins.jl … origin/main …mutate.jl --check; bounded--warmhot_path.jl; selected rungit diff; method sweepsrc/changePer-file counts: positional encoding 152; MHA 106; architecture builders 81; forward inference 68;
SympNet 145; attention 66; Bool parameters 70; VPFF 66; constructor bugs 14; Aqua 17; JET 52 plus one
Broken; exports 8; reachability 2; docs-reference checks 6. Scoped 1.12.7 run: 407/407.
Full suite, 1.13.2: PASS, exit 0, ran to the end (
Testing GeometricMachineLearning tests passed),6821 pass + 1 broken = 6822, no failing testset; Metal 40/40 on an Apple M4 Max. Full suite, 1.12.7:
PASS, exit 0, ran to the end, 6821 pass + 1 broken = 6822, the same testsets row for row. (1.12 runs
--check-bounds=yes, so the guarded@allocatedassertions are skipped there; that run is notevidence for an allocation bound.)
Pre-PR verification
julia-branch-verifierran twice, once atddb908d8and once at75e1ba85.Pass 1 (3 unresolved, all text, all settled):
parent commitandstillphrasings of four test files wererewritten, and the
positional_encoding(backend, T, dim, seq_length)method the branch added onan exported function is now documented. The cited path
test/layers/symplectic_attention_network_parameters_gradient.jlexists, and the family claimholds (
SymplecticAutoencoder <: SymplecticCompression <: AutoEncoder).MultiHeadAttentiondraw-order entry moved to Changed (breaking), and the
upscaling_activationrename keeps oneentry.
file:linecitations checked; K21 stays recorded (see below).@docsblock andtest/integration/documented_defaults.jlfollow them; no
act,upsacling,init_bias!,:bias,:skew_symorEncoderInit…remainsin
src/,docs/src/or the tests.test/runtests.jlandJLArraysis intest/Project.toml;architectures/gradient_layer_builders.jlran 81 pass andlayers/positional_encoding.jl152 pass on 1.13.2.@assert typeof(Chain(arch).layers[i]) <: …{3, 3, <Bool>, …}lines evaluateto
truein a session. No docs build was run locally, so the added docstring lines are checked bythe Documentation workflow.
@show,println,@infoorTODO, and no scratch file in the diff.Pass 2: Unresolved: 0.
Transformerdocstring as well, because the diff removesthe "makes the network CPU-only" sentence from both.
leaves nothing dangling.
quality/doctests.jl—doctest(GeometricMachineLearning)— PASS, 88 s, at75e1ba85.Pass 2's two edits, and two of mine — K21's per-pass figure, which the pass called a nit, and the
K22 entry it reported as a pre-existing defect — are in
df6f2416.Unresolved: 1. K21 records the allocation comparison's order sensitivity instead of making the
measurement independent of earlier allocations. That is a decision taken above the part, and the
tolerance is unchanged.
Advisor decisions
NonLinearSymplecticEncoder/NonLinearSymplecticDecoder,PSDArchkeepsUnknownSymplectic…,PlainAEgivesUnknown….abs(measured - literal) ≤ 1024B, controlliteral > 0.restoration and the extraction pins, the corrected 192 B/pass figure, and the plan-wide pin rule.