Skip to content

Add bilinear and low-rank velocity-parity Stiefel layers and trace loss - #368

Merged
michakraus merged 4 commits into
mainfrom
multimodal-symplectic-ae/part-e
Oct 29, 2026
Merged

michakraus merged 4 commits into
mainfrom
multimodal-symplectic-ae/part-e

Conversation

@michakraus

Copy link
Copy Markdown
Member

Summary

Add two-sided velocity-parity bilinear Stiefel layers, dynamical low-rank approximation (DLRA) Stiefel layers, a fast low-rank trace loss, and Tucker-2 HOSVD initialization to GeometricMachineLearning:

  • BilinearStiefelLayer(Nx, Nv, mx, mv; metric_factors = nothing) and TransposeBilinearStiefelLayer (inverse_layer(l)): split a 2D (Nx, Nv) or 3D (Nx, Nv, B) phase-space array into velocity-even and velocity-odd components $F^+, F^- \in \mathbb{R}^{N_x \times (N_v / 2)}$ (or accept a pre-split (q, p) NamedTuple), apply optional Cholesky mass factors (Rx, Rv), and project onto $\Phi_x \in \mathrm{St}(m_x, N_x)$ and $\Phi_v \in \mathrm{St}(m_v, N_v / 2)$ to produce canonical coordinates (q = vec(Q_red), p = vec(P_red)) of half-dimension $n = m_x m_v$.
  • LowRankStiefelLayer(Nx, Nv, mx, mv; r_out = mx * mv, metric_factors = nothing) and DecodeLowRankStiefelLayer (inverse_layer(l)): encode a factored DLRA tuple (X = X, S = S, V = V) of arbitrary rank $r_{\mathrm{in}} \ge 1$ in $O(r_{\mathrm{in}}(m_x N_x + m_v N_v))$ operations without assembling the dense $N_x \times N_v$ matrix $X S V^T$, matching BilinearStiefelLayer on $F = X S V^T$ identically and remaining invariant under $O(r_{\mathrm{in}}) \times O(r_{\mathrm{in}})$ gauge transformations; decode via the truncated SVD of $[Q_{\mathrm{red}};; P_{\mathrm{red}}]$ of rank $r_{\mathrm{eff}} = \min(r_{\mathrm{out}}, m_x, 2 m_v)$ followed by QR re-orthonormalization (_thin_qr_cgs) to return (X = X_hat, S = S_hat, V = V_hat) on the Stiefel manifold.
  • FastDLRATraceLoss(): evaluate $|X S V^T - \hat{X} \hat{S} \hat{V}^T|_F^2$ in $O(r^2(N_x + N_v))$ operations via explicit Gram-matrix trace contractions clamped at zero(T).
  • solve!(l::BilinearStiefelLayer, ps, F_snapshots) and solve!(nn::NeuralNetwork, F_snapshots): initialize ps.Φx and ps.Φv via truncated Tucker-2 HOSVD (svd(...; full = true)) on spatial and parity-velocity snapshot unfoldings.
  • Add docs/src/layers/stiefel_layer.md and test/layers/bilinear_and_low_rank_stiefel.jl.

Stacked on #367 (Part D).

Pre-PR verification

Verdict: clear to open — 0 unresolved
Diff: 9 files, +1862/−1, 3 commits base b144023...head 0bf0e73
Scope: full seven points

Fixed in this pass

# point file:line what was wrong what changed
— — — none none

Unresolved — the PR does not open until these are answered

# severity file:line claim evidence
— — — none none

Pre-existing, adjacent — fix in this change?

# severity file:line claim evidence
— — — none none

Checked and clean

  • 1 · correctness, quality, concision: read all 9 changed files (src/layers/bilinear_stiefel_layer.jl:1-497, src/layers/low_rank_stiefel_layer.jl:1-354, src/loss/dlra_trace_loss.jl:1-125, src/GeometricMachineLearning.jl:251-275, docs/make.jl:199-203, docs/src/layers/stiefel_layer.md:1-23, test/layers/bilinear_and_low_rank_stiefel.jl:1-857, test/runtests.jl:28, CHANGELOG.md:151) and checked against test/layers/layer_structure.jl, test/quality/exports.jl, test/quality/reachability.jl, and docs/check_references.jl. All type constructors use ::Type{T}, all 6 new docstrings appear in docs/src/layers/stiefel_layer.md with no blank/comment line before the documented definition, 3D batched contractions use batched BLAS GEMM (reshape/permutedims and tensor_tensor_mul/tensor_transpose_tensor_mul), DecodeLowRankStiefelLayer uses functional classical Gram-Schmidt (_thin_qr_cgs) for Zygote reverse-mode AD compatibility, and solve! uses svd(...; full = true) to handle rank-deficient unfoldings.
  • 2 · no duplicated functionality: checked against AbstractNeuralNetworks, NeuralNetworkParameters, GeometricOptimizers, and ReducedComplexityModeling; velocity-parity bilinear Stiefel projections, DLRA factor layers, and FastDLRATraceLoss do not duplicate dependency APIs.
  • 3 · type piracy (static check): inspected every method definition in src/layers/bilinear_stiefel_layer.jl, src/layers/low_rank_stiefel_layer.jl, and src/loss/dlra_trace_loss.jl. Every method added to an imported function (AbstractNeuralNetworks.inverse_layer, AbstractNeuralNetworks.parameterlength, AbstractNeuralNetworks.initialparameters, ReducedComplexityModeling.solve!) dispatches on a package-owned type (BilinearStiefelLayer, TransposeBilinearStiefelLayer, LowRankStiefelLayer, DecodeLowRankStiefelLayer, or NeuralNetwork{..., <:Chain{<:Tuple{BilinearStiefelLayer, Vararg}}}).
  • 4 · type instability (static check): verified concrete type parameters and promote_type(eltype(x), eltype(ps.Wx), ...) / inv(sqrt(T(2))) / zero(T) arithmetic across all forward, decode, and loss paths, plus @inferred assertions in test/layers/bilinear_and_low_rank_stiefel.jl on all constructors, inverse_layer, initialparameters, and 2D/3D calls.
  • 5 · avoidable allocations (static check): ran fatou lint --no-config on all 7 changed .jl files (src/layers/bilinear_stiefel_layer.jl, src/layers/low_rank_stiefel_layer.jl, src/loss/dlra_trace_loss.jl, src/GeometricMachineLearning.jl, test/layers/bilinear_and_low_rank_stiefel.jl, test/runtests.jl, docs/make.jl) with 0 findings on new or modified hunks. Confirmed @view usage on array slices and two-batch-size scaling assertions (B1 = 16, B2 = 64) in test/layers/bilinear_and_low_rank_stiefel.jl:804-855.
  • 6 · stale comments, present tense: read every docstring and comment in src/layers/bilinear_stiefel_layer.jl, src/layers/low_rank_stiefel_layer.jl, src/loss/dlra_trace_loss.jl, docs/src/layers/stiefel_layer.md, and test/layers/bilinear_and_low_rank_stiefel.jl; all prose is in the present tense and matches the implementation.
  • 7 · history only in CHANGELOG.md: grepped all changed files for temporal/history phrases (previously, used to, was renamed, no longer, as of, before this, originally, recently, has been changed to, going forward, from now on) with 0 hits outside CHANGELOG.md, and verified the ## [Unreleased] entry at CHANGELOG.md:151.

Not checked

  • 3 · type piracy (live Aqua.test_piracies): Kaimon live session creation was disabled because the Kaimon server had stalled sessions (test/quality/aqua.jl runs Aqua.test_all(GeometricMachineLearning; ambiguities = false) in CI).
  • 4 · type instability (live code_typed): Kaimon live session creation was disabled (@inferred covers all new layers and loss paths in test/layers/bilinear_and_low_rank_stiefel.jl).
  • 5 · avoidable allocations (live cold-process @allocated): Julia cannot run directly in the terminal sandbox and Kaimon session creation was disabled.

Changelog: entry present
Release: minor bump — adds exported BilinearStiefelLayer, TransposeBilinearStiefelLayer, LowRankStiefelLayer, DecodeLowRankStiefelLayer, and FastDLRATraceLoss (0.x repository, covered by the pending 0.9.0 release in CHANGELOG.md)
Follow-up measurement: none

Critic

Verdict: PASS on 0bf0e734 (Round: verify 3)

clause test or command verdict
1 · BilinearStiefelLayer and TransposeBilinearStiefelLayer (inverse_layer), initialparameters, velocity-parity split/recombine, optional metric_factors, right-inverse identity to $30,\mathrm{eps}(T)$, parity $L^2$ isometry to $20,\mathrm{eps}(T)$, Kronecker symplecticity to $30,\mathrm{eps}(T)$ test/layers/bilinear_and_low_rank_stiefel.jl (BilinearStiefelLayer constructor, guards, parameters and BilinearStiefelLayer forward, inverse_layer, parity isometry, Kronecker symplecticity) PASS
2 · LowRankStiefelLayer and DecodeLowRankStiefelLayer (inverse_layer), arbitrary input rank $r_{\mathrm{in}} \ge 1$, cross-modal equivalence with BilinearStiefelLayer to $30,\mathrm{eps}(T)$, SVD + QR Stiefel decoder with $|\hat{X}^T \hat{X} - I|_F \le 30,\mathrm{eps}(T)$, $|\hat{V}^T \hat{V} - I|_F \le 30,\mathrm{eps}(T)$, and dense equivalence to $50,\mathrm{eps}(T)$ test/layers/bilinear_and_low_rank_stiefel.jl (LowRankStiefelLayer and DecodeLowRankStiefelLayer) PASS
3 · FastDLRATraceLoss() $O(r^2(N_x + N_v))$ Gram-matrix trace identity clamped at zero(T), matching dense Frobenius norm to $100,\mathrm{eps}(T),|X S V^T|_F^2$ even on approximately orthonormal factors test/layers/bilinear_and_low_rank_stiefel.jl (FastDLRATraceLoss) PASS
4 · solve!(nn::NeuralNetwork, F_snapshots) and solve!(l::BilinearStiefelLayer, ps, F_snapshots) Tucker-2 HOSVD initialization matching optimal subspace projection on rank-$(m_x, m_v)$ synthetic data to $50,\mathrm{eps}(T)$ test/layers/bilinear_and_low_rank_stiefel.jl (solve! Tucker-2 HOSVD initialization on BilinearStiefelLayer) PASS
5 · Docstrings in docs/src/, @inferred, Aqua, ExplicitImports, GML test suite, CHANGELOG.md, and batched Zygote differentiation / allocation scaling docs/src/layers/stiefel_layer.md, CHANGELOG.md, test/layers/bilinear_and_low_rank_stiefel.jl (Batched Zygote differentiation and allocation scaling) PASS

Known issues added

none

Decisions taken without the user

  • Decided (arbitrator, 2026-10-26): The direct Cayley path of part A (cayley_direct!, DifferentiatedCayleyTransport) solves its $2k \times 2k$ system with lu!/ldiv!. On Metal it runs under allowscalar(false): the reviewer of the pull request measured both against the host and got differences of about 1e-7 and 8e-7 in Float32. On a JLArray it raises "Scalar indexing is disallowed.", because JLArrays has no lu. No clause of this plan runs it on a device, and part A has no device test (it would go in test/devices/metal.jl). A part that puts a direct Stiefel method on a device measures it on Metal through Kaimon, and never reads device support from a JLArray test.
  • Decided (arbitrator, 2026-10-26; user, 2026-10-26): A test of a refusing guard asserts the guard's whole message with an anchored pattern (r"^…$"), not a substring. A phrase such as "a matrix of size" also occurs in Base's own DimensionMismatch of a matrix product, so a substring test passes when the guard is removed and a later operation raises.
  • Decided (arbitrator, 2026-10-29): Batched 3D DecodeLowRankStiefelLayer maps _decode_dlra_2d over eachcol(vcat(reshape(Q_red, Mx * Mv, B), reshape(P_red, Mx * Mv, B))) rather than indexing Q_red[:, :, b] and P_red[:, :, b] inside map(1:B), avoiding $B$ 3D getindex pullbacks and $O(B^2)$ backward allocations. Batched 3D _dlra_trace_loss_3d uses tensor_transpose_tensor_mul, tensor_tensor_mul, and whole-array trace reductions sum(M .* permutedims(G, (2, 1, 3)); dims = (1, 2)) with sum(max.(zero(T), t1 .- T(2) .* tc .+ t2)), deleting _batched_at_b, _batched_a_b, _batched_a_bt, and the duplicate ChainRulesCore.rrule(::typeof(_dlra_trace_loss_3d), ...).

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 25 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

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: 8157744c-ccb7-4296-8530-8dee1dc23bf5
Commits

Reviewing files that changed from the base of the PR and between 1e47d17 and aedf828.

Files selected for processing: 1
  • test/layers/bilinear_and_low_rank_stiefel.jl
📝 Summary

Summary by CodeRabbit

  • New Features
    • Added layers for converting phase-space data and low-rank states into compact coordinates and reconstructing them, with optional metric weighting.
    • Added a loss function for comparing low-rank states and snapshot-based initialization for bilinear layers.
  • Documentation
    • Added a guide to the new Stiefel layers.
  • Tests
    • Added coverage for layer behavior, reconstruction, loss calculations, and initialization.

The change adds bilinear and low-rank Stiefel layers, a DLRA trace loss, and snapshot-based basis initialization. It exports the new types, documents them, and adds tests for their behavior.

Changes: Stiefel layers and DLRA loss

Layer / File(s) Summary
Bilinear parity projection and reconstruction
src/layers/bilinear_stiefel_layer.jl, src/GeometricMachineLearning.jl, docs/src/layers/stiefel_layer.md, docs/make.jl, CHANGELOG.md, test/layers/bilinear_and_low_rank_stiefel.jl, test/runtests.jl
The bilinear encoder and transpose decoder split fields into velocity-parity components, project and reconstruct them through Stiefel bases, and accept optional metric factors. The package exports the new types and adds documentation and tests.
Snapshot-based basis initialization
src/layers/bilinear_stiefel_layer.jl, test/layers/bilinear_and_low_rank_stiefel.jl
The layer and network solve! methods initialize bases from snapshot singular vectors. The network methods can copy matching encoder bases to a final transpose decoder.
Low-rank encoding and decoding
src/layers/low_rank_stiefel_layer.jl, test/layers/bilinear_and_low_rank_stiefel.jl
The encoder maps DLRA factors to parity coordinates. The decoder reconstructs factors using truncated SVD and QR, with optional metric transformations. Tests cover encoding, decoding, gradients, and allocation behavior.
Factor-based DLRA trace loss
src/loss/dlra_trace_loss.jl, test/layers/bilinear_and_low_rank_stiefel.jl
FastDLRATraceLoss computes reconstruction error from factor Gram matrices for 2D and batched 3D inputs. Callable methods support factor states, model predictions, and neural networks.

Priority: Normal

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

Change: Feature

Possibly related PRs

Merge Risk: High · previous review 1e47d

Resolve the failing allocation test and memory-risk initialization before merging; correct the documented complexity bounds as well.

Pre-merge checks: Passed 5
Check name Status Explanation
Description check Passed The description directly explains the new bilinear and low-rank Stiefel layers, trace loss, HOSVD initialization, documentation, and validation covered by the changeset.
Title check Passed The title clearly and concisely identifies the main additions: bilinear and low-rank velocity-parity Stiefel layers and the trace loss.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (4 skipped: 4 unsupported.)

  • 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

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

@michakraus
michakraus force-pushed the multimodal-symplectic-ae/part-d branch from b144023 to 8e28bb2 Compare October 29, 2026 15:26
Base automatically changed from multimodal-symplectic-ae/part-d to main October 29, 2026 17:15

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


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: 98d7d193-0237-4c4a-8862-a9b8249be2d9
Commits

Reviewing files that changed from the base of the PR and between 8433513 and 0bf0e73.

Files selected for processing: 57
  • CHANGELOG.md
  • KNOWN_ISSUES.md
  • Project.toml
  • docs/inventories/GeometricOptimizers.toml
  • docs/make.jl
  • docs/src/architectures/transformer.md
  • docs/src/data_loader/data_loader.md
  • docs/src/data_loader/snapshot_matrix.md
  • docs/src/introduction.md
  • docs/src/layers/stiefel_layer.md
  • docs/src/layers/symmetric_particle_layers.md
  • docs/src/tutorials/symplectic_autoencoder.md
  • docs/src/tutorials/sympnet_tutorial.md
  • scripts/Project.toml
  • scripts/reproduction/symplectic_autoencoders/online_sympnet.jl
  • scripts/utilities/parametric_data_helpers.jl
  • src/GeometricMachineLearning.jl
  • src/architectures/forced_sympnet.jl
  • src/architectures/parametric_resnet.jl
  • src/architectures/symmetric_symplectic_autoencoder.jl
  • src/architectures/sympnet.jl
  • src/architectures/transformer_integrator.jl
  • src/architectures/transformer_neural_network.jl
  • src/data_loader/batch.jl
  • src/data_loader/data_loader.jl
  • src/data_loader/parametric_data_loader.jl
  • src/data_loader/tensor_assign.jl
  • src/data_loader/train.jl
  • src/layers/bilinear_stiefel_layer.jl
  • src/layers/canonical_slot_layer.jl
  • src/layers/canonical_sort_layer.jl
  • src/layers/low_rank_stiefel_layer.jl
  • src/layers/pairwise_symplectic_layer.jl
  • src/loss/dlra_trace_loss.jl
  • src/loss/losses.jl
  • src/parametric.jl
  • test/Project.toml
  • test/architectures/parametric_resnet.jl
  • test/data_loader/accuracy.jl
  • test/data_loader/autoencoder_keyword.jl
  • test/data_loader/batch.jl
  • test/data_loader/batch_functor.jl
  • test/data_loader/batch_index_set.jl
  • test/data_loader/data_loader.jl
  • test/data_loader/draw_batch_for_tensor_test.jl
  • test/data_loader/parametric_data_loader.jl
  • test/data_loader/train.jl
  • test/integration/docstrings/data_loader.jl
  • test/integration/qp_dispatch.jl
  • test/layers/bilinear_and_low_rank_stiefel.jl
  • test/layers/symmetric_particle_layers.jl
  • test/optimizers/optimizer.jl
  • test/parametric.jl
  • test/quality/aqua.jl
  • test/quality/exports.jl
  • test/quality/jet.jl
  • test/runtests.jl
Files with no reviewable changes: 11
  • test/integration/docstrings/data_loader.jl
  • test/data_loader/draw_batch_for_tensor_test.jl
  • test/data_loader/accuracy.jl
  • test/data_loader/autoencoder_keyword.jl
  • docs/src/architectures/transformer.md
  • src/data_loader/tensor_assign.jl
  • test/data_loader/batch_index_set.jl
  • test/data_loader/batch_functor.jl
  • src/data_loader/parametric_data_loader.jl
  • src/data_loader/data_loader.jl
  • src/data_loader/batch.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.

Comment on lines +453 to +463
# Spatial mode-1 unfolding: Nx × (Nv * n_snaps)
M_x = hcat(reshape(Fp_w, Nx, half_Nv * n_snaps), reshape(Fm_w, Nx, half_Nv * n_snaps))
svd_x = svd(M_x; full = true)
_copy_stiefel!(ps.Φx, @view(svd_x.U[:, 1:Mx]))

# Parity-velocity mode-2 unfolding: (Nv ÷ 2) × (2 * Nx * n_snaps)
M_v = hcat(
reshape(permutedims(Fp_w, (2, 1, 3)), half_Nv, Nx * n_snaps),
reshape(permutedims(Fm_w, (2, 1, 3)), half_Nv, Nx * n_snaps))
svd_v = svd(M_v; full = true)
_copy_stiefel!(ps.Φv, @view(svd_v.U[:, 1:Mv]))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major · Performance & Scalability · Quick win

Do not use svd(...; full = true) on the snapshot unfoldings. It allocates a square matrix the size of the column count.

svd(M_x; full = true) returns the full right factor V. Its size is (Nv * n_snaps) × (Nv * n_snaps). For M_v, the full right factor has size (2 * Nx * n_snaps) × (2 * Nx * n_snaps). Only U[:, 1:Mx] and U[:, 1:Mv] are used.

Example with Nx = Nv = 64 and n_snaps = 200:

  • M_v has 25600 columns.
  • Its full V needs about 5.2 GB.
  • The snapshot data itself is only about 6.5 MB.

With realistic snapshot counts, solve! therefore runs out of memory or stalls.

full = true only matters in the rank-deficient case, where mx or mv exceeds the number of columns. That case is covered at test lines 657-670. Use a thin SVD for the normal case. In the rank-deficient case, complete the basis with QR. With this change, memory stays proportional to size(M).

🐛 Proposed fix
--- "a/src/layers/bilinear_stiefel_layer.jl"
+++ "b/src/layers/bilinear_stiefel_layer.jl"
@@ -450,17 +450,22 @@
     F_plus, F_minus = _parity_split(F_snapshots)
     Fp_w, Fm_w = _apply_metric_forward_3d(F_plus, F_minus, l.metric_factors)
 
+function _leading_left_singular_vectors(M::AbstractMatrix, m::Int)
+    F = svd(M)  # thin SVD: U is N × min(N, ncols)
+    size(F.U, 2) >= m && return F.U[:, 1:m]
+    # Rank-deficient unfolding: complete span(U) to an orthonormal N × m basis.
+    return qr(F.U).Q * Matrix{eltype(F.U)}(I, size(M, 1), m)
+end
+
     # Spatial mode-1 unfolding: Nx × (Nv * n_snaps)
     M_x = hcat(reshape(Fp_w, Nx, half_Nv * n_snaps), reshape(Fm_w, Nx, half_Nv * n_snaps))
-    svd_x = svd(M_x; full = true)
-    _copy_stiefel!(ps.Φx, @view(svd_x.U[:, 1:Mx]))
+    _copy_stiefel!(ps.Φx, _leading_left_singular_vectors(M_x, Mx))
 
     # Parity-velocity mode-2 unfolding: (Nv ÷ 2) × (2 * Nx * n_snaps)
     M_v = hcat(
         reshape(permutedims(Fp_w, (2, 1, 3)), half_Nv, Nx * n_snaps),
         reshape(permutedims(Fm_w, (2, 1, 3)), half_Nv, Nx * n_snaps))
-    svd_v = svd(M_v; full = true)
-    _copy_stiefel!(ps.Φv, @view(svd_v.U[:, 1:Mv]))
+    _copy_stiefel!(ps.Φv, _leading_left_singular_vectors(M_v, Mv))
     return ps
 end
 

Define the helper at file scope, outside solve!.

Comment thread src/layers/canonical_sort_layer.jl Outdated
Comment on lines +184 to +185
inverse_layer(::CanonicalSortLayer{M, Axis}) where {M, Axis} = InvertCanonicalSortLayer{M}()
inverse_layer(::InvertCanonicalSortLayer{M}) where {M} = CanonicalSortLayer{M, 1}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Functional Correctness · Quick win

inverse_layer does not round-trip for Axis != 1.

InvertCanonicalSortLayer{M} does not store Axis. Line 185 therefore maps it back to CanonicalSortLayer{M, 1}. inverse_layer(inverse_layer(CanonicalSortLayer{4, 2}())) returns CanonicalSortLayer{4, 1}(). The result sorts along the wrong coordinate row, and no error is raised.

SymmetricAutoencoderLayer avoids this by building the encoder's sort layer from arch.sort_axis. Other callers that apply inverse_layer to a decoder chain do not get this protection. The pairwise layers round-trip, and the tests only cover the default axis (lines 59-61 in the test file).

Document this limitation in the InvertCanonicalSortLayer docstring. Alternatively, carry Axis as a second type parameter. If you document it, add one sentence to the docstring:

📝 Proposed docstring note
-When called on `(q, p)` or a flattened array without a stored `perm`, it acts as the identity on the canonical Weyl chamber.
+When called on `(q, p)` or a flattened array without a stored `perm`, it acts as the identity on the canonical Weyl chamber.
+Note that `inverse_layer(::InvertCanonicalSortLayer)` returns `CanonicalSortLayer{M, 1}`, because the sort axis is not stored. Construct `CanonicalSortLayer{M, Axis}` explicitly for `Axis != 1`.

Comment on lines +13 to +14
const N_INDEPENDENCE_TOLERANCE = 4096
n_independent(a::Real, b::Real) = abs(a - b) <= N_INDEPENDENCE_TOLERANCE

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major · Functional Correctness · Quick win

Make n_independent a one-sided check. The absolute per-sample tolerance fails because of fixed overhead, not because of superlinear growth.

The pipeline failure is at Line 854. CI reports these per-sample values:

  • alloc_full_bwd2 / B2 ≈ 76168
  • alloc_full_bwd1 / B1 ≈ 80315

The gap is about 4147 bytes, which exceeds N_INDEPENDENCE_TOLERANCE = 4096. The large batch allocates less per sample than the small batch, so allocation does not grow superlinearly.

The cause is fixed overhead. Total allocation follows c + s * B. The per-sample value is then c / B + s. Between B1 = 16 and B2 = 64, the per-sample gap is c * (1/16 - 1/64). That gap exceeds 4096 bytes when c is above about 87 KB. The Zygote pullback for the full DLRA chain reaches that size.

The same test is fragile at Lines 812, 818, 832, 840, and 846. Any growth in fixed overhead, such as a new Julia or Zygote version, fails these assertions too.

The test should only reject per-sample growth with batch size. Every call site passes the B2 value first, so a one-sided comparison fixes all assertions.

💚 Proposed fix
--- "a/test/layers/bilinear_and_low_rank_stiefel.jl"
+++ "b/test/layers/bilinear_and_low_rank_stiefel.jl"
@@ -10,8 +10,10 @@
 
 include("../helpers/eltypes.jl")
 
 const N_INDEPENDENCE_TOLERANCE = 4096
-n_independent(a::Real, b::Real) = abs(a - b) <= N_INDEPENDENCE_TOLERANCE
+# `a` is the per-sample allocation at the larger batch and `b` at the smaller batch.
+# Fixed overhead amortizes, so only reject per-sample growth with batch size.
+n_independent(a::Real, b::Real) = a <= b + N_INDEPENDENCE_TOLERANCE
 
 function _random_stiefel(rng, ::Type{T}, N::Int, m::Int) where {T <: Real}
     A = randn(rng, T, N, m)

Also applies to: 848-854

Source: Pipeline failures

@michakraus
michakraus force-pushed the multimodal-symplectic-ae/part-e branch from 0bf0e73 to 1e47d17 Compare October 29, 2026 18:51

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


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: bd024aeb-21a5-42ab-82a5-844dce9d0517
Commits

Reviewing files that changed from the base of the PR and between 0bf0e73 and 1e47d17.

Files selected for processing: 4
  • CHANGELOG.md
  • docs/make.jl
  • src/GeometricMachineLearning.jl
  • test/runtests.jl
Files skipped from review as they are similar to previous changes: 1
  • docs/make.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.

Comment thread CHANGELOG.md

- **`CanonicalSortLayer`, `InvertCanonicalSortLayer`, `PairwiseSymplecticLayer`, `PairwiseSymplecticLayerQ`, `PairwiseSymplecticLayerP`, `InversePairwiseSymplecticLayerQ`, `InversePairwiseSymplecticLayerP`, `CanonicalSlotLayer`, `InverseCanonicalSlotLayer`, and `SymmetricSymplecticAutoencoder` add particle symplectic layers that are invariant or equivariant under relabelling of the particles, and a weight-tied symplectic autoencoder with three particle frontends.** All ten names are exported. `CanonicalSortLayer(M, d, axis = 1)` makes `CanonicalSortLayer{N, M, Axis}` with `N = 2Md`, sorts `M` particles in ``\mathbb{R}^d`` along coordinate row `axis` of `q` using a stable merge sort (`MergeSort`), applies the same permutation ``\sigma \in S_M`` to both `q` and `p`, and returns `(q = q_sorted, p = p_sorted, perm = σ)` on `(q, p)` `NamedTuple` inputs of shape `(d, M)` or `(d, M, B)` (or the sorted state on flattened `2Md` vectors and `(2Md, B)` matrices). `InvertCanonicalSortLayer(M, d)` makes `InvertCanonicalSortLayer{N, M}` and applies ``\sigma^{-1}`` (`invperm`) to restore the original particle ordering; `inverse_layer` pairs it with `CanonicalSortLayer`. `CanonicalSortLayer`, `InvertCanonicalSortLayer`, `CanonicalSlotLayer` and `InverseCanonicalSlotLayer` carry the phase-space dimension of the flattened state as their first type parameter, `N = 2Md`, so `input_dimension` and `output_dimension` return the length of that state, and their inputs are checked against the fixed `d`, with the first dimension of an input exactly its input dimension. `PairwiseSymplecticLayerQ(M, d, upscale, σ)` and `PairwiseSymplecticLayerP(M, d, upscale, σ)` apply ``S_M``-equivariant canonical shears combining a self term ``K_{\mathrm{self}}^T \sigma(K_{\mathrm{self}} u_i + b_{\mathrm{self}})`` and a skew-symmetric pairwise relative potential term ``\frac{1}{M}\sum_{j=1}^M K_{\mathrm{pair}}^T \tau(K_{\mathrm{pair}}(u_i - u_j))`` with ``\tau(x) = \sigma(x) - \sigma(-x)``, conserving total canonical momentum (or position) when ``K_{\mathrm{self}} = 0`` and pairing with `InversePairwiseSymplecticLayerQ` and `InversePairwiseSymplecticLayerP` through `inverse_layer`. `CanonicalSlotLayer(M, d, NSlots)` (`M >= 2`, `1 <= NSlots <= M`, `CanonicalSlotLayer{N, NOut, M, NSlots}` with `NOut = 2 NSlots d`) first sorts the particles along coordinate row 1 of `q` with `CanonicalSortLayer`, so its slots are invariant under a relabelling of particles with distinct first coordinates. It then separates the center-of-mass mode ``(Q_{\mathrm{cm}}, P_{\mathrm{cm}}) = \frac{1}{\sqrt{M}}\sum_{i=1}^M (q_i, p_i)`` from the ``M - 1`` zero-sum Helmert relative modes, sorts the ``M - 1`` relative modes canonically by descending squared phase-space norm ``\|q_{\mathrm{rel}, k}\|_2^2 + \|p_{\mathrm{rel}, k}\|_2^2`` (`MergeSort`), and projects the sorted relative modes onto ``N_{\mathrm{slots}} - 1`` slots via a learned `StiefelManifold` parameter ``\Phi_{\mathrm{rel}} \in \mathrm{St}(N_{\mathrm{slots}} - 1, M - 1)`` (`NSlots == 1` keeps only the center-of-mass slot with no parameter leaf). `InverseCanonicalSlotLayer(M, d, NSlots)` takes the same type parameters, with input dimension `NOut` and output dimension `N`, and lifts the slots back symplectically via ``\Phi_{\mathrm{rel}}`` and the transpose Helmert matrix. `SymmetricSymplecticAutoencoder(M, d, reduced_dim; frontend = :sort | :pairwise | :slot, ...) <: SymplecticCompression` combines one of the three particle frontends with the weight-tied `WeightTiedSymplecticAutoencoder` core. With `:sort` and `:slot` the latent state is invariant under a relabelling of particles with distinct coordinates along the sorting axis (`sort_axis` for `:sort`, coordinate 1 for `:slot`). With `:pairwise` only the frontend is permutation-equivariant, because the core is not, so the latent state is not invariant. Right-inversion ``\mathrm{encoder}(nn)(\mathrm{decoder}(nn)(\xi)) = \xi`` holds to machine precision for every ``\xi`` with `:pairwise`; with `:sort` and `:slot` it holds only for ``\xi`` whose decoded state lies in the canonical chamber, where the sorting permutations on the decoded state are the identity, because the decoder does not map into that chamber in general. The tests are `test/layers/symmetric_particle_layers.jl`.

- **`BilinearStiefelLayer`, `TransposeBilinearStiefelLayer`, `LowRankStiefelLayer`, `DecodeLowRankStiefelLayer`, `FastDLRATraceLoss`, and `solve!` on `BilinearStiefelLayer` add two-sided velocity-parity Stiefel frontends, a gauge-invariant dynamical low-rank Stiefel frontend and decoder, a fast low-rank trace loss, and Tucker-2 HOSVD initialization.** The five new types are exported. `BilinearStiefelLayer(Nx, Nv, mx, mv; metric_factors = nothing)` (`Nx >= 1`, even `Nv >= 2`, `1 <= mx <= Nx`, `1 <= mv <= Nv ÷ 2`) allocates `(Φx = StiefelManifold(...), Φv = StiefelManifold(...))` with ``\Phi_x \in \mathrm{St}(m_x, N_x)`` and ``\Phi_v \in \mathrm{St}(m_v, N_v / 2)``, splits an input array ``F`` of shape `(Nx, Nv)` or `(Nx, Nv, B)` into velocity-even ``F^+ = \frac{1}{\sqrt{2}}(F_{:, 1:N_v/2} + F_{:, N_v:-1:N_v/2+1})`` and velocity-odd ``F^- = \frac{1}{\sqrt{2}}(F_{:, 1:N_v/2} - F_{:, N_v:-1:N_v/2+1})`` components (or accepts a pre-split `(q, p)` `NamedTuple` of shape `(Nx, Nv ÷ 2)` or `(Nx, Nv ÷ 2, B)`), applies optional Cholesky mass factors `(Rx, Rv)` when `metric_factors` is supplied, and projects ``Q_{\mathrm{red}} = \Phi_x^T F^+ \Phi_v`` and ``P_{\mathrm{red}} = \Phi_x^T F^- \Phi_v`` to return vectorized `(q = vec(Q_red), p = vec(P_red))` of half-dimension ``n = m_x m_v``; `TransposeBilinearStiefelLayer` (`inverse_layer(l)`) is its exact right-inverse. `LowRankStiefelLayer(Nx, Nv, mx, mv; r_out = mx * mv, metric_factors = nothing)` takes a DLRA `NamedTuple` `(X = X, S = S, V = V)` of arbitrary rank ``r_{\mathrm{in}} \ge 1`` and evaluates ``Q_{\mathrm{red}} = (\Phi_x^T X) S ((V^+)^T \Phi_v)`` and ``P_{\mathrm{red}} = (\Phi_x^T X) S ((V^-)^T \Phi_v)`` in ``O(r_{\mathrm{in}}(m_x N_x + m_v N_v))`` operations without forming ``X S V^T``, matching `BilinearStiefelLayer` on ``F = X S V^T`` identically and remaining invariant under ``O(r_{\mathrm{in}}) \times O(r_{\mathrm{in}})`` gauge transformations, while `DecodeLowRankStiefelLayer` (`inverse_layer(l)`) computes the truncated SVD of ``[Q_{\mathrm{red}}\;\; P_{\mathrm{red}}]`` of rank ``r_{\mathrm{eff}} = \min(r_{\mathrm{out}}, m_x, 2 m_v)`` and QR re-orthonormalizes the lifted factors to return `(X = X_hat, S = S_hat, V = V_hat)` on the Stiefel manifold. `FastDLRATraceLoss()` evaluates ``\|X S V^T - \hat{X} \hat{S} \hat{V}^T\|_F^2`` in ``O(r^2(N_x + N_v))`` operations via Gram-matrix traces clamped at `zero(T)`. `solve!(nn, F_snapshots)` and `solve!(l, ps, F_snapshots)` initialize `Φx` and `Φv` on a `BilinearStiefelLayer` from the top-``m_x`` spatial and top-``m_v`` parity-velocity left singular vectors of `F_snapshots`. The tests are `test/layers/bilinear_and_low_rank_stiefel.jl`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Performance & Scalability · Quick win

Correct the rank-complexity claims.

LowRankStiefelLayer performs rank-space products such as Hx * S, adding terms such as O(m_x r_in^2). FastDLRATraceLoss forms products such as S' * Gx11 * S, adding O(r^3) work, or O(B r^3) for batches. Update the bounds or state a fixed-rank assumption. (raw.githubusercontent.com)

@codecov

codecov Bot commented Oct 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.94595% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.45%. Comparing base (67c48f9) to head (aedf828).

Files with missing lines Patch % Lines
src/layers/low_rank_stiefel_layer.jl 94.18% 10 Missing ⚠️
src/layers/bilinear_stiefel_layer.jl 96.69% 7 Missing ⚠️
src/loss/dlra_trace_loss.jl 98.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #368      +/-   ##
==========================================
+ Coverage   95.32%   95.45%   +0.12%     
==========================================
  Files          58       61       +3     
  Lines        1820     2264     +444     
==========================================
+ Hits         1735     2161     +426     
- Misses         85      103      +18     

☔ 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
michakraus enabled auto-merge October 29, 2026 20:13
@michakraus
michakraus disabled auto-merge October 29, 2026 20:36
@michakraus
michakraus merged commit 9ef9680 into main Oct 29, 2026
14 of 17 checks passed
@michakraus
michakraus deleted the multimodal-symplectic-ae/part-e branch October 29, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant