From 2f9a50efb7509b496acdbaf9aa78ee7a441e4401 Mon Sep 17 00:00:00 2001 From: Michael Kraus Date: Sun, 25 Oct 2026 14:20:52 +0100 Subject: [PATCH 1/2] Correct the comments and docstrings that known issues K1, K11, K18, K22, K25, K26 and K27 name Comment, docstring and manual text only; no code behaviour changes. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 2 + KNOWN_ISSUES.md | 100 ------------------ docs/src/data_loader/data_loader.md | 2 +- src/architectures/autoencoder.jl | 4 +- .../linear_symplectic_transformer.jl | 2 +- src/architectures/symplectic_autoencoder.jl | 2 +- src/architectures/symplectic_transformer.jl | 2 +- src/layers/symplectic_attention.jl | 2 +- .../adam_with_learning_rate_decay.jl | 3 +- test/reduced_system/reduced_system.jl | 2 +- 10 files changed, 11 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2b07c98d..ba2999f81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,8 @@ breaking release). - **A `(q, p)` input whose `p` has the wrong size raises the layer's own `ErrorException("Dimension mismatch.")`.** The forward methods of `ActivationLayer`, `GradientLayer` and `LinearLayer` checked the size of `q` and not the size of `p`, so a `p` of the wrong size raised whichever error its first operation produced: a `DimensionMismatch` from a matrix product or a broadcast, or an `AssertionError` from the `SymmetricMatrix` product. Each method now checks `q` and `p` together. No valid input changes. This closes known issue K23, which leaves `KNOWN_ISSUES.md`. The test is `test/layers/layer_structure.jl`, testset "a SympNet layer raises on a half of the wrong size". +- **This closes known issues K1, K11, K18, K22, K25, K26 and K27, which leave `KNOWN_ISSUES.md`.** Comments, docstrings and the DataLoader manual now state the current code: the reduced-system test names C21 in `KNOWN_ISSUES.md`; the `AdamOptimizerWithDecay` test states the present constraint; the symplectic attention comment cites the real test path; the transformer docstrings give the gradient layers of one unit as the chain builds them; the `SymplecticAutoencoder` docstring says `decoder_init_q` gives a q-type first layer; the `UnknownDecoder` docstring names `n_decoder_blocks` and links `UnknownEncoder`; and the tensor-sampling formula gains the `+ 1` of its upper bound. No behaviour changes, and C21 stays open. + ### Added - **`KernelAbstractions.get_backend` is defined for `PoissonTensor`.** GeometricOptimizers 0.9 asks every operand of a checked product for its backend, so `𝕁 * Y` with a `StiefelManifold` raised `ArgumentError: Implement KernelAbstractions.get_backend(::PoissonTensor...)` without it. diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md index e4b75673f..f476dae34 100644 --- a/KNOWN_ISSUES.md +++ b/KNOWN_ISSUES.md @@ -68,30 +68,6 @@ What is known to be wrong in GeometricMachineLearning and is not fixed. ## Found late -### K1 · A test comment sends the reader to an Open Issues section of `CHANGELOG.md`, which does not exist. - -- **location:** `test/reduced_system/reduced_system.jl:44` -- **kind:** found late -- **found:** 2026-09-26 -- **evidence:** - ``` - $ grep -rnE 'Open Issues|issue C1[48]|\*C21\*|\*B7\*' src test - test/reduced_system/reduced_system.jl:44:# for four of twelve, with four `NaN` and four in the opposite order. That is *C21* in - ``` - - The ID is still valid; the comment at `test/reduced_system/reduced_system.jl:44–45` names - `CHANGELOG.md` rather than `KNOWN_ISSUES.md`. The fix is a follow-up change to that comment. - -### K11 · A comment in `adam_with_learning_rate_decay.jl` reports a past experiment. - -- **location:** `test/optimizers/adam_with_learning_rate_decay.jl` -- **kind:** found late -- **found:** 2026-10-12, by the critics of part M1a; the comment is older than that part -- **evidence:** - The comment above `train_manifold_network` says "from a seeded start the loss fell by under 2% - on 1.13 and *rose* on 1.12". The fix states only the present constraint: a 32-epoch budget - drives the learning rate to `η₂` almost at once, and 128 epochs train. - ### K17 · `assign_q_and_p` and `PoissonTensor * v` drop the rows from `2N + 1` on without an error. - **location:** `src/kernels/assign_q_and_p.jl` @@ -154,66 +130,6 @@ What is known to be wrong in GeometricMachineLearning and is not fixed. `src/data_loader/data_loader.jl:205` states the one-type signature. Closing it means widening the three signatures to `NamedTupleOfArrays{T, (:q, :p)}`, and the docstring with them. -### K22 · A comment in `symplectic_attention.jl` names a test file under `test/parameters/`, which does not exist. - -- **location:** `src/layers/symplectic_attention.jl:131` -- **kind:** docs -- **found:** 2026-10-20, by the critics of part M5; the comment is the same on `origin/main` at `4c4a8320` -- **evidence:** - The comment above the symmetric forward method of `SymplecticAttention` names - `test/parameters/symplectic_attention_network_parameters_gradient.jl`. The file is - `test/layers/symplectic_attention_network_parameters_gradient.jl`, and `test/parameters/` does not - exist. The fix changes the path in the comment. - -### K25 · The docstrings of `LinearSymplecticTransformer` and `SymplecticTransformer` say that `n_sympnet = 1` gives one `GradientLayerQ` and one `GradientLayerP`. - -- **location:** `src/architectures/linear_symplectic_transformer.jl:22` -- **kind:** docs -- **found:** 2026-10-21, by the branch verifier of part M5; the docstrings and the chains are the same on `origin/main` at `4c4a8320` -- **evidence:** - Each transformer unit puts the same `n_sympnet` gradient layers after its `Q` attention layer and - after its `P` attention layer, and the alternation starts again in each block. With - `n_sympnet = 1` and the default `init_upper = true`, both gradient layers are `GradientLayerQ`: - - ```julia - [typeof(l) for l in Chain(LinearSymplecticTransformer(4, 3; n_sympnet = 1)).layers] - # LinearSymplecticAttentionQ, GradientLayerQ, LinearSymplecticAttentionP, GradientLayerQ - [typeof(l) for l in Chain(SymplecticTransformer(4; n_sympnet = 1)).layers] - # SymplecticAttentionQ, GradientLayerQ, SymplecticAttentionP, GradientLayerQ - ``` - - The same sentence is at `src/architectures/symplectic_transformer.jl:35`. The count - `2n_sympnet` holds for one unit, `L = 1`. The fix states the layers of one unit as the chain - builds them, or changes the chain, which changes the network. - -### K26 · The docstring of `SymplecticAutoencoder` says that `decoder_init_q = true` makes the first layer of each decoder block a ``p``-type layer, where it is a `GradientLayerQ`. - -- **location:** `src/architectures/symplectic_autoencoder.jl:36` -- **kind:** docs -- **found:** 2026-10-21, by the branch verifier of part M5; the docstring is the same on `origin/main` at `4c4a8320` -- **evidence:** - `decoder_init_q = true` selects `:DecoderInitQ`, whose `decoder_layers_from_iteration` passes - `isodd`, so layer 1 of each decoder block is a `GradientLayerQ`: - - ```julia - ae = NeuralNetwork(SymplecticAutoencoder(8, 4; decoder_init_q = true), CPU(), Float64) - first(decoder(ae).model.layers) isa GradientLayerQ # true - ``` - - The fix writes ``q`` for ``p`` in the docstring. - -### K27 · The docstring of `UnknownDecoder` names its third argument `n_encoder_blocks` and links to itself for an example. - -- **location:** `src/architectures/autoencoder.jl:102` -- **kind:** docs -- **found:** 2026-10-21, by the branch verifier of part M5; the docstring is the same on `origin/main` at `4c4a8320` -- **evidence:** - The signature line reads `UnknownDecoder(full_dim, reduced_dim, n_encoder_blocks)`, and the third - argument fills the field `n_decoder_blocks` (`src/architectures/autoencoder.jl:113`). The last - line says that an example "can be constructed analogously to [`UnknownDecoder`](@ref)", which - links the docstring to itself; the example is in the docstring of `UnknownEncoder`. The fix names - the argument `n_decoder_blocks` and links `UnknownEncoder`. - ## Upstream ### K2 · Revise prints EMFILE errors in the test log @@ -252,22 +168,6 @@ What is known to be wrong in GeometricMachineLearning and is not fixed. itself records a skip where `Metal.functional()` is false; only the precompile fails. Closing this is a Metal.jl change: a workload that does not run where no device exists. -## Documentation - -### K18 · The tensor-sampling formula in the `DataLoader` manual is one short - -- **location:** `docs/src/data_loader/data_loader.md:143` -- **kind:** docs -- **found:** 2026-10-16 -- **evidence:** - Item 1 of *Sampling from a tensor* gives the start positions as - `shuffle(1:(input_time_steps − seq_length − prediction_window))`. For the time-series loader - without an output shown on that page, `number_of_start_positions` takes - `n = input_time_steps − seq_length − prediction_window + 1`, and `number_of_batches` counts - `cld(n · n_params, batch_size)` minibatches (`src/data_loader/batch.jl`). For the example's - five time steps and `Batch(4, 2, 0)`, the formula gives three start positions per parameter - and the code gives four. The correction is to add `+ 1` to the formula's upper bound. - ## The test suite ### K3 · `test/quality/jet.jl` does not see an unstable kernel-launch argument, nor two barriers in the Poisson-tensor kernel diff --git a/docs/src/data_loader/data_loader.md b/docs/src/data_loader/data_loader.md index 86251f7c8..1e3cfe344 100644 --- a/docs/src/data_loader/data_loader.md +++ b/docs/src/data_loader/data_loader.md @@ -140,7 +140,7 @@ batch(dl) ``` Sampling from a tensor is done the following way (``\mathcal{I}_i`` again denotes the batch indices for the ``i``-th batch): -1. ``\mathtt{time\_indices} \leftarrow \mathtt{shuffle}(\mathtt{1:}(\mathtt{input\_time\_steps} - \mathtt{seq\_length} - \mathtt{prediction\_window}),`` +1. ``\mathtt{time\_indices} \leftarrow \mathtt{shuffle}(\mathtt{1:}(\mathtt{input\_time\_steps} - \mathtt{seq\_length} - \mathtt{prediction\_window} + 1),`` 2. ``\mathtt{parameter\_indices} \leftarrow \mathtt{shuffle}(\mathtt{1:n\_params}),`` 3. ``\mathtt{complete\_indices} \leftarrow \mathtt{product}(\mathtt{time\_indices}, \mathtt{parameter\_indices}),`` 3. ``\mathcal{I}_i \leftarrow \mathtt{complete\_indices[}(i - 1) \cdot \mathtt{batch\_size} + 1 : i \cdot \mathtt{batch\_size]}\text{ for }i=1, \ldots, (\mathrm{last} -1),`` diff --git a/src/architectures/autoencoder.jl b/src/architectures/autoencoder.jl index 0801cdf36..c992b09e6 100644 --- a/src/architectures/autoencoder.jl +++ b/src/architectures/autoencoder.jl @@ -99,13 +99,13 @@ struct UnknownEncoder <: Encoder end """ - UnknownDecoder(full_dim, reduced_dim, n_encoder_blocks) + UnknownDecoder(full_dim, reduced_dim, n_decoder_blocks) Make an instance of `UnknownDecoder`. This should be used if one wants to use an [`Decoder`](@ref) that does not have any specific structure. -An example of using this can be constructed analogously to [`UnknownDecoder`](@ref). +An example of using this can be constructed analogously to [`UnknownEncoder`](@ref). """ struct UnknownDecoder <: Decoder full_dim::Int diff --git a/src/architectures/linear_symplectic_transformer.jl b/src/architectures/linear_symplectic_transformer.jl index 180963d89..3bbc0e70a 100644 --- a/src/architectures/linear_symplectic_transformer.jl +++ b/src/architectures/linear_symplectic_transformer.jl @@ -19,7 +19,7 @@ You can provide the additional optional keyword arguments: raw""": The activation function for the SympNet layers. - `init_upper::Bool=true`: Specifies if the first layer is a ``q``-type layer (`init_upper=true`) or if it is a ``p``-type layer (`init_upper=false`). -The number of SympNet layers in the network is `2n_sympnet`, i.e. for `n_sympnet = 1` we have one [`GradientLayerQ`](@ref) and one [`GradientLayerP`](@ref). +Each transformer unit has `n_sympnet` gradient layers after its ``q`` attention layer and `n_sympnet` after its ``p`` attention layer, so one unit has `2n_sympnet` SympNet layers. In each group of `n_sympnet` layers the type alternates between [`GradientLayerQ`](@ref) and [`GradientLayerP`](@ref), starting with the type that `init_upper` selects. For `n_sympnet = 1` and `init_upper = true` each group is one [`GradientLayerQ`](@ref). """ struct LinearSymplecticTransformer{AT} <: TransformerIntegrator where {AT} dim::Int diff --git a/src/architectures/symplectic_autoencoder.jl b/src/architectures/symplectic_autoencoder.jl index 694713a91..e3410bd79 100644 --- a/src/architectures/symplectic_autoencoder.jl +++ b/src/architectures/symplectic_autoencoder.jl @@ -33,7 +33,7 @@ Besides the required arguments `full_dim` and `reduced_dim` you can provide the - `sympnet_upscale::Integer = 5`: The *upscaling dimension* of the GSympNet. See [`GradientLayerQ`](@ref) and [`GradientLayerP`](@ref). - `activation = tanh`: The activation in the gradient layers. - `encoder_init_q::Bool = true`: Specifies if the first layer in each encoder block should be of $q$ type. -- `decoder_init_q::Bool = true`: Specifies if the first layer in each decoder block should be of $p$ type. +- `decoder_init_q::Bool = true`: Specifies if the first layer in each decoder block should be of $q$ type. """ struct SymplecticAutoencoder{EncoderInit, DecoderInit, AT} <: SymplecticCompression full_dim::Int diff --git a/src/architectures/symplectic_transformer.jl b/src/architectures/symplectic_transformer.jl index e87688c17..f52d0a5aa 100644 --- a/src/architectures/symplectic_transformer.jl +++ b/src/architectures/symplectic_transformer.jl @@ -32,7 +32,7 @@ You can provide the additional optional keyword arguments: - `symmetric::Bool = """ * "$(st_symmetric_default)`" * raw""": Specifies whether the weighting matrix ``A`` of the [`SymplecticAttention`](@ref) layers is symmetric (`symmetric=true`) or arbitrary (`symmetric=false`). -The number of SympNet layers in the network is `2n_sympnet`, i.e. for `n_sympnet = 1` we have one [`GradientLayerQ`](@ref) and one [`GradientLayerP`](@ref). +Each transformer unit has `n_sympnet` gradient layers after its ``q`` attention layer and `n_sympnet` after its ``p`` attention layer, so one unit has `2n_sympnet` SympNet layers. In each group of `n_sympnet` layers the type alternates between [`GradientLayerQ`](@ref) and [`GradientLayerP`](@ref), starting with the type that `init_upper` selects. For `n_sympnet = 1` and `init_upper = true` each group is one [`GradientLayerQ`](@ref). """ struct SymplecticTransformer{SAT, AAT, Upscaling} <: TransformerIntegrator dim::Int diff --git a/src/layers/symplectic_attention.jl b/src/layers/symplectic_attention.jl index c94125c4e..1c3463e73 100644 --- a/src/layers/symplectic_attention.jl +++ b/src/layers/symplectic_attention.jl @@ -128,7 +128,7 @@ end # `A = ps.A` once, and then `A` twice: the local binding is what keeps the `SymmetricMatrix` # structure in the Zygote gradient of this layer. Read `ps.A` twice and the gradient leaf degrades -# to a plain `Matrix` -- `test/parameters/symplectic_attention_network_parameters_gradient.jl` +# to a plain `Matrix` -- `test/layers/symplectic_attention_network_parameters_gradient.jl` # asserts both halves of that, and the last testset there covers this method. The binding costs no # allocation; it is the number of accesses that decides the structure. function (d::SymplecticAttention{M, M, LayerType, :symmetric})( diff --git a/test/optimizers/adam_with_learning_rate_decay.jl b/test/optimizers/adam_with_learning_rate_decay.jl index eac788f52..c4ad1ee80 100644 --- a/test/optimizers/adam_with_learning_rate_decay.jl +++ b/test/optimizers/adam_with_learning_rate_decay.jl @@ -49,8 +49,7 @@ an ordinary `Adam` and the manifold weights take GeometricOptimizers' Adam cache """ # `n_epochs = 128` and not 32: `AdamOptimizerWithDecay(n_epochs)` fixes # γ = exp(log(η₂/η₁)/n_epochs), so a 32-epoch budget drives the learning rate to η₂ = 1e-6 almost -# at once and the run barely trains -- from a seeded start the loss fell by under 2% on 1.13 and -# *rose* on 1.12. Over 128 epochs the decay is gentle enough that it falls by a factor of six. +# at once and the run barely trains. Over 128 epochs the decay is gentle enough that the run trains. function train_manifold_network(; n_epochs = 128) Random.seed!(123) arch = Chain(StiefelLayer(1, 20), Dense(20, 20, tanh), Dense(20, 1, identity)) diff --git a/test/reduced_system/reduced_system.jl b/test/reduced_system/reduced_system.jl index 35534e7e2..2e53f40b9 100644 --- a/test/reduced_system/reduced_system.jl +++ b/test/reduced_system/reduced_system.jl @@ -42,7 +42,7 @@ end # comes back `NaN` and the comparison is not between two numbers. At the seed above it is `NaN`. # Measured over seeds 1 to 12: `ImplicitMidpoint` orders the two for every seed, `ExplicitMidpoint` # for four of twelve, with four `NaN` and four in the opposite order. That is *C21* in -# `CHANGELOG.md`. +# `KNOWN_ISSUES.md`. function test_reduced_vector_fields(reduced_dim::Integer, integrator; compare_reduction_error = true) rs1, rs2 = set_up_reduced_systems(reduced_dim, integrator) From b40df1e12178ea54dfa45690a540d76d05aef406 Mon Sep 17 00:00:00 2001 From: Michael Kraus Date: Sun, 25 Oct 2026 14:28:44 +0100 Subject: [PATCH 2/2] Close the shuffle call in the tensor-sampling formula of the DataLoader manual Co-Authored-By: Claude Sonnet 5.5 --- docs/src/data_loader/data_loader.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/data_loader/data_loader.md b/docs/src/data_loader/data_loader.md index 1e3cfe344..2cb8a7bd7 100644 --- a/docs/src/data_loader/data_loader.md +++ b/docs/src/data_loader/data_loader.md @@ -140,7 +140,7 @@ batch(dl) ``` Sampling from a tensor is done the following way (``\mathcal{I}_i`` again denotes the batch indices for the ``i``-th batch): -1. ``\mathtt{time\_indices} \leftarrow \mathtt{shuffle}(\mathtt{1:}(\mathtt{input\_time\_steps} - \mathtt{seq\_length} - \mathtt{prediction\_window} + 1),`` +1. ``\mathtt{time\_indices} \leftarrow \mathtt{shuffle}(\mathtt{1:}(\mathtt{input\_time\_steps} - \mathtt{seq\_length} - \mathtt{prediction\_window} + 1)),`` 2. ``\mathtt{parameter\_indices} \leftarrow \mathtt{shuffle}(\mathtt{1:n\_params}),`` 3. ``\mathtt{complete\_indices} \leftarrow \mathtt{product}(\mathtt{time\_indices}, \mathtt{parameter\_indices}),`` 3. ``\mathcal{I}_i \leftarrow \mathtt{complete\_indices[}(i - 1) \cdot \mathtt{batch\_size} + 1 : i \cdot \mathtt{batch\_size]}\text{ for }i=1, \ldots, (\mathrm{last} -1),``