Losses and pullbacks: one default loss, one pullback shape, and the symbolic HNN gradient (B5) by michakraus · Pull Request #344 · JuliaGNI/GeometricMachineLearning.jl

michakraus · 2026-10-14T10:40:15Z

Summary

Losses and pullbacks, part M1b of the 0.9 simplification of GeometricOptimizers and GeometricMachineLearning. Relates to #145.

One default-loss mechanism. train!'s default is loss = NetworkLoss(nn.architecture, batch). A TransformerIntegrator with a Batch{:Transformer} gets TransformerLoss(batch). A NeuralNetworkIntegrator or an AutoEncoder with a Batch{:FeedForward} gets NetworkLoss(arch): FeedForwardLoss() through the new NetworkLoss(::NeuralNetworkIntegrator), and AutoEncoderLoss(). Any other architecture and batch raise an ArgumentError that asks for loss. NetworkLoss(::TransformerIntegrator, prediction_window::Integer) is removed, so a transformer's prediction window comes from the Batch it trains on. _default_loss is gone.
loss takes a NetworkLoss or an AbstractPullback. The pullback keyword is removed. train! builds the pullback once per call, with _as_pullback: a NetworkLoss trains through a ZygotePullback, and an AbstractPullback through itself.
One pullback shape. ZygotePullback's back(Δ) is a NetworkParameters with keys(ps), not a one-tuple, as SymbolicPullback's is. A structured leaf holds its storage gradient, so a SymmetricMatrix leaf comes back as a SymmetricMatrix whose off-diagonal entries are twice those of the projection of the dense gradient (issue Zygote gradient w.r.t. NetworkParameters loses the SymmetricMatrix leaf on a second access #319). Both methods take ps::NetworkParameters; a bare NamedTuple reaches AbstractNeuralNetworks' untyped fallback, which raises an ErrorException. _processing, _get_contents, _get_params and _only_set are deleted.
A loss functor takes a NetworkParameters. The Union{NetworkParameters, NamedTuple} arm of HNNLoss, LNNLoss, SymplecticEulerLoss and VariationalMidpointLoss is gone, and AutoEncoderLoss, TransformerLoss and ReducedLoss annotate theirs ::NetworkParameters.
The symbolic HNN pullback is the gradient of HNNLoss on the batch (issue SymbolicPullback(arch) gives the gradient of the per-sample sum, not of HNNLoss #343). SymbolicPullback(arch) returns the value ‖R‖/‖Y‖ and the gradient Δ · Σᵢ ∇θ ½‖rᵢ‖² / (‖R‖‖Y‖): the generated per-sample gradient of the surrogate, summed with build_nn_function(…; reduce = +) and scaled by the two norms of the value's residual. A call evaluates the Hamiltonian vector field once. Each norm divides each gradient entry on its own, because ‖R‖‖Y‖ overflows, and its inverse underflows, where neither norm does. At a zero residual the scale is pinv(‖R‖), so the gradient is zero, as ZygotePullback's is. The gradient has the parameters' element type whatever the data's. The pullback is the upstream type SymbolicPullback(loss, fun) with a call method of this package for a private fun type. CHANGELOG.md quotes the gradient before and after against Zygote's.
The four-argument AutoEncoderLoss ignores its target and reconstructs its input, so AutoEncoderLoss()(model, ps, x, y) == AutoEncoderLoss()(model, ps, x).
Known issues. B5, K6 and K13 leave KNOWN_ISSUES.md. C7 is rewritten: the symbolic pullback reaches one private SymbolicNeuralNetworks name, symbolic_parameter_gradient. K14 is added: train! infers its history as an abstract Vector, although its run-time element type is the loss value's. Relates to Storing parameters AbstractNeuralNetworks.jl#15.
Tests. test/pullbacks/zygote_pullback.jl, test/pullbacks/symbolic_hnn_pullback.jl and test/loss/losses.jl are new, and test/data_loader/train.jl and test/loss/training_method_losses.jl are rewritten. test/helpers/leaves.jl holds the loop over the leaves of a gradient. Every numeric testset runs over REAL_ELTYPES, asserts value isa T and the element type of every gradient leaf, and seeds Random. The symbolic pullback's tests compare value and gradient with ZygotePullback(HNNLoss(arch)) at batch sizes 1 and 5, at the overflow and underflow scales, at a zero residual, for a seed of either float type and for data of the other float type.

Fixes #319.
Fixes #343.

🤖 Generated with Claude Code

Both ZygotePullback methods take ps::NetworkParameters and return (value, Δ -> only(pb(Δ))), so `back` hands one parameter set back as SymbolicPullback's already does. `_processing`, `_get_contents`, `_get_params` and `_only_set` are deleted with their test, the three `@docs` lines and the doctest's `_processing` line. The `::Tuple` annotation on the pullback call is dropped: it lowers to `convert(Tuple, ex)::Tuple`, the identity on a tuple, so it neither widens nor narrows the body and only implied a guarantee the type system does not hold.

The `Union{NetworkParameters, NamedTuple}` arm of HNNLoss, LNNLoss, SymplecticEulerLoss and VariationalMidpointLoss goes: a bare NamedTuple is a MethodError rather than a silently different gradient, which is what the arm hid. The ReducerLoss comment that said AD hands the loss a bare NamedTuple is false under NeuralNetworkParameters 0.5 and goes; the three texts that repeat it are rewritten to the rule.

`train!`'s default is `NetworkLoss(nn.architecture, batch)`: the two methods live in `src/data_loader/train.jl` because `Batch` is included after `src/loss/`, and any other architecture reaches an ArgumentError that names `loss`. The old `NetworkLoss(::TransformerIntegrator, prediction_window = 1)` is gone -- against `Batch`'s default of `seq_length` it was the conflict, and the batch now sets both lengths. The `loss` keyword takes a NetworkLoss or an AbstractPullback and the `pullback` keyword goes, so a call that passes it is a MethodError. `_pullback` dispatches once per call, not once per step.

Three testsets of `test/data_loader/train.jl` are rewritten and `test/loss/losses.jl` is new, all in `core`. The default-loss testset now walks `NetworkLoss(arch, batch)` rather than `_default_loss`; the `_pullback` testset counts three calls for twelve samples at Batch(4); the HNN testset passes `loss = SymbolicPullback(arch)` and its `@test_broken` becomes `@test`s at rtol = sqrt(eps(T)). The two `ps`-typing testsets assert the MethodError on a bare NamedTuple.

The generated per-sample gradient of the surrogate ½‖rᵢ‖² is scaled by 1/(‖R‖‖Y‖) from one forward pass more than the loss value needs, so the symbolic gradient is the gradient of HNNLoss on the batch rather than of the per-sample sum. The two norms are divided by separately and never multiplied: their product overflows at about 1.8e19 in Float32 where neither does. At a zero residual the scale is pinv(‖R‖), which is zero, so the gradient is the zero set as Zygote's is. `ParameterGradient` is no longer used, so it leaves NON_PUBLIC_QUALIFIED; the reach is one private SymbolicNeuralNetworks name, `symbolic_parameter_gradient", and the false comment and every sentence saying the batch loss is a sum of per-sample losses go.

`core` gains pullbacks/zygote_pullback.jl, pullbacks/symbolic_hnn_pullback.jl and loss/losses.jl, and loses pullbacks/get_contents.jl. The tutorial's `CustomLoss` annotates its `ps` a NetworkParameters, which is what a loss takes now.

The [Unreleased] section gains M1b's Removed, Changed and Fixed entries, and the M1a `train!` bullet is corrected in place to `loss = NetworkLoss(nn.architecture, batch)`, which accepts a pullback. B5, C7, K6 and K13 leave KNOWN_ISSUES.md. K14 is new: edge 10 case (b) holds, so `train!` infers its history as an abstract Vector in both precisions while its run-time element type is T, and every test asserts that. The M1a verifier's finding #3 moves there.

The three `TransformerLoss` methods and the model-slot methods of `HNNLoss`, `LNNLoss`, `SymplecticEulerLoss` and `VariationalMidpointLoss` keep an untyped `ps`, so a bare `NamedTuple` reaches them and returns a loss value, which is what a gradient of another shape does silently. The four two-argument methods already annotate theirs; these seven now do too.

…otocol The two-field constructor `SymbolicPullback(loss, fun)` and the call protocol `fun(input, output, ps)(Δ)` are documented under *Implementation*, and the sentence about the upstream construction says what it does instead of claiming a corrected dimension. Each of the two norms now divides a gradient entry rather than the seed: the one scalar `1/(‖R‖‖Y‖)` goes subnormal in `Float32` from a scale of about `1e40`, where the entries stay of order one. The seed is converted to the parameters' element type, so a `Float64` seed on a `Float32` network gives `Float32` leaves, as `ZygotePullback`'s does. The comment about a `Float64` literal widening a `Float32` network goes: the in-place kernel that `build_nn_function` builds writes into an array of the promoted element type, and `/ 2` as `0.5 *` is an equivalent mutant.

Edge 11 and §V want every numeric testset to assert `value isa T` and that every gradient leaf has element type `T`: the symbolic pullback's extreme-scale, zero-residual and seed testsets, the `ZygotePullback` testsets, `AutoEncoderLoss`'s and the Hamiltonian testset of `train!` now do. The first `ZygotePullback` testset checks every primal leaf's size, and its `SymmetricMatrix` row uses an `LA` SympNet; the symbolic pullback draws its data with `randn(Xoshiro(seed), T, 2, N)`. The extreme-scale testset gains a row at which the seed scalar underflows while the gradient does not, which is what the two separate divisions buy, and the seed testset checks a seed of the other float type. `_pullback` is counted through a test-local method on a `NetworkLoss`, so the count is of `train!`'s builds rather than of its steps; the `AbstractPullback`'s body is counted separately, so a `_pullback` that wraps one in a `ZygotePullback` is caught too. `TransformerLoss` joins the `NetworkParameters` testset, and every comment that carried history about this release is in the present tense.

`Zygote.pullback` keeps a `SymmetricMatrix` leaf for a `NetworkParameters` at two accesses, where it drops the bare `NamedTuple`'s to a `Matrix`, and `ProjectTo` of a `NetworkParameters` is a projector onto that type, not the identity.

…e becomes `Any` C7 is rewritten to the one private name `SymbolicNeuralNetworks.symbolic_parameter_gradient`, with the reason edge 6 gives and the constructor and call protocol that are documented. K14 no longer names a `::Tuple` annotation this branch removed, no longer claims an inference barrier restores the type, and measures `Batch(8)`: `DataLoader`'s minibatch is a pair of 3-tensors, the same call on matrices infers `Float32`, and the first `Any` of `@code_warntype` is in `Zygote.pullback(f, ::NetworkParameters)`. The CHANGELOG moves the symbolic-HNN gradient under *Fixed*, keeps the branch's own ExplicitImports note rather than M1a's Aqua one, no longer claims it closes C7 or K14, and carries the notes for the numbers that stay vacant.

The CHANGELOG's migration note for the removed keyword names 0.8; the docstring states the present signature.

…entry Each norm divided a gradient entry through a closure that recomputed `norm(output)` and `pinv(norm_R)` for every leaf and formed one intermediate array per leaf. `inv_R`, `norm_Y` and `TP` are now taken once per `fun(input, output, ps)` call and `c = convert(TP, Δ)` once per seed, and the leaf map `x -> x .* c .* inv_R ./ norm_Y` is one fused broadcast: one array per leaf, with the same per-element operations in the same order. Measured against the previous closure in a cold process, `@allocated back(one(value))` after one warm-up call: 1744 to 1232 bytes in `Float32` and 2336 to 1680 in `Float64`. Every leaf is `==` the previous closure's, bitwise, for N = 1 and 5 at the `Y` scales 1, `1e20`/`1e155` and `1e23`/`1e170` in both precisions. The comment no longer says that the gradient entries stay of order one where the seed scalar underflows: at the test's own `Float32` scale `1e23` the generated entries reach `1.36e22` and the gradient entries `1.64e-25`. It says instead that the per-entry divisions keep every leaf finite and non-zero there.

Edge 11 asks every numeric testset for `value isa T`; the shape testset of `ZygotePullback` and the seed testset of `SymbolicPullback` asserted only the gradient's element types and now assert the value's too. The four testsets of the symbolic file build their network with the global RNG unseeded, so the network's own weights drifted between runs; each now seeds it, with a distinct seed per testset, as `zygote_pullback.jl` does. The over-50-seed run of the file's assertions passed either way, so the seeding pins what was already robust rather than repairing a failure. The underflow row's comment no longer says the gradient entries stay of order one at the second scale, where the scalar `1/(‖R‖‖Y‖)` is zero: it says that the per-entry divisions keep every leaf finite and non-zero.

…oves C7 is rewritten, not removed, so the sentence that listed it among the removed entries of `KNOWN_ISSUES.md` was false. It now names B5, K6 and K13 as the removed entries and says that the notes those two entries carried for the numbers that stay vacant go with them: B1, B2, B3, B4, B6, B8 and B9 left with B5, and C9, C12 and C13 with C7. The norms sentence no longer says the seed scalar goes subnormal past `1e40`. `1/floatmin(Float32)` is `8.5e37`, so the scalar is subnormal once the product of the two norms passes that; each norm divides every gradient entry on its own, and the product of the two norms overflows in `Float32` where neither norm does.

K14 quoted the `Any` of Zygote's own IR — `%5 = ZygoteRulesExt.Any::Core.Const(Any)` — which is a statement about Zygote's lowering rather than about the call chain. It now carries the chain `k14_chain.jl` measures, and quotes its 3-tensor rows for both precisions: the forward loss infers `Float32`, `Zygote._pullback(Context(), f, ps)` and `Zygote.pullback(f, ps)` give `Any`, and so does `Zygote.pullback(z -> sum(model(z, ps)), x)` on the 3-tensor alone, which puts the widening in Zygote's adjoint of the layer rather than in the loss or the wrapper. NeuralNetworkParameters reaches that pullback through `invoke(ZygoteRules.pullback, Tuple{Any, Vararg{Any}}, f, ps)` at its `ext/ZygoteRulesExt.jl:25`.

The round-1b reproducer of edge 10 (b) asserts that K14 names a `@code_warntype` call and no longer names a `::Tuple` annotation the branch removed. The measured chain is now introduced as the first `Any` of `@code_warntype` on `Zygote.pullback(f, ::NetworkParameters)`, which is the one inside Zygote's own pullback, rather than as a free-standing statement; the `Core.Const(Any)` quote of Zygote's IR stays out, and the round-1b testsets of edge 8, edge 10 (b) and edge 6 pass.

… names The loss part removes `ParameterGradient` from the ignore list of the two ExplicitImports checks, so the count of non-public qualified accesses this package reaches falls from ten to nine. K14's evidence now names `ext/ZygoteRulesExt.jl:25`, so the location field holds that path, attributed to the package that owns it, instead of a dash; and the entry no longer tells the reader to run a script that is not in the repository.

…ameters The pullback part adds `parameter_eltype` to the import, so `mapstorage` is no longer the one thing that comes from there rather than being written again here.

The scale `1/(‖R‖‖Y‖)` is not one at `N = 1`, where the batch is one sample and the sum is a single per-sample gradient; the batch of five is where the per-sample sum's gradient and the batch loss's gradient differ.

codecov · 2026-10-14T11:08:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.77%. Comparing base (3484c6a) to head (60cc906).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   88.49%   88.77%   +0.27%     
==========================================
  Files          74       74              
  Lines        2633     2636       +3     
==========================================
+ Hits         2330     2340      +10     
+ Misses        303      296       -7

☔ 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 of `0cb4e21fffa265d6dc4dbc87f32ceb140b9f1dc8`

Verdict: request changes. Posted as a comment review, because the PR is authored by the account that posts it.

Focus, as asked: fragile, verbose, inefficient and overcomplicated code, and where the new code can be drastically simplified. Every finding carries the command or the file:line that produced it. The probes ran cold against a scratch environment that develops this head: AbstractNeuralNetworks 0.9.1, NeuralNetworkParameters 0.5.2, SymbolicNeuralNetworks 0.9.0, GeometricOptimizers 0.9.0, Zygote 0.7.13, Julia 1.13.1.

CI

All seven required checks pass (Julia min/1 × ubuntu/macOS/windows, Doctests). Downgrade - ubuntu-latest fails, and it is not required: neither the classic protection of main nor its ruleset lists it, and .github/workflows/CI.yml:118-135 marks the job continue-on-error: true ("advisory"). Its log is a resolver failure, not a test failure. forcedeps check failed for GPUArraysCore (resolved 0.2.1, floor 0.2.0), Zygote (0.7.5 / 0.7.0), ChainRulesCore (1.25.1 / 1.0.0), NNlib (0.9.30 / 0.8.0) and KernelAbstractions (0.9.38 / 0.9.0), and the suite never ran. The same job fails with the same five lines on main at 3484c6a7 (run 38596013154, today). This PR does not touch Project.toml. The failure is pre-existing and not attributable to this PR.

Findings

#	severity	file:line	claim
1	bug	`src/pullbacks/symbolic_hnn_pullback.jl:82-87`	the symbolic pullback breaks `train!` for a `Float32` network on `Float64` data
2	bug	`CHANGELOG.md:42`	"a bare `NamedTuple` is a `MethodError`" is false for `ZygotePullback`
3	quality	`src/pullbacks/symbolic_hnn_pullback.jl:57-88`	the value and the scale each evaluate the vector field; one evaluation is enough
4	quality	`src/pullbacks/symbolic_hnn_pullback.jl:75-87`	the per-entry underflow guard protects a regime where the reference already fails
5	quality	`src/data_loader/train.jl:69-77`	the default loss excludes two architectures that have a one-argument `NetworkLoss`
6	quality	`src/loss/losses.jl:145-157`	the four-argument `AutoEncoderLoss` drops its target silently, and the methods route through each other
7	quality	`src/data_loader/train.jl:64-65`, `test/data_loader/train.jl:194-210`	`_pullback` collides with the `_pullback` argument name, and a test extends it to count one line
8	quality	`test/loss/losses.jl:11,30`, `test/pullbacks/zygote_pullback.jl:67`, `test/data_loader/train.jl:173`, `test/helpers/network_parameters_gradient_structure.jl:19`	comments cite "decision 8/9/32/34", a document that is not in the repository
9	nit	`src/pullbacks/symbolic_hnn_pullback.jl:49-53`	construction builds the symbolic network and its vector field twice
10	nit	`test/pullbacks/symbolic_hnn_pullback.jl:35,75,110,134`	the file builds the same `SymbolicPullback` twelve times
11	nit	`src/data_loader/train.jl:35-36`	the comment describes a closure and a per-step alternative that do not exist
12	nit	`CHANGELOG.md:54-66`	process bookkeeping in a user-facing changelog
13	nit	`src/pullbacks/symbolic_hnn_pullback.jl:8-46,66,79-81`, `docs/src/architectures/hamiltonian_neural_network.md:59`	long docstring about a private callable; a source comment cites a test constant; `<: Function` does nothing
14	nit	PR body	internal process records and unresolved link labels

1 · bug: the symbolic pullback breaks `train!` for a `Float32` network on `Float64` data

_HNNParameterGradient converts the seed to the parameters' element type (c = convert(TP, Δ), :86). But inv_R and norm_Y have the data's element type, and the generated kernel promotes as well. So the leaves come back in the promoted type, while ZygotePullback returns the parameters' type (probe_correctness.jl, P1):

P1 net=Float32 data=Float64 leaf eltypes symbolic => DataType[Float64]
P1 net=Float32 data=Float64 leaf eltypes zygote   => DataType[Float32]

Through train!, the symbolic path raises and the default path trains (probe_mixed_train.jl: StandardHamiltonianArchitecture(2, 4, 1), NeuralNetwork(arch, CPU(), Float32), DataLoader(randn(2, 8), randn(2, 8)), GradientMethod):

symbolic => (error = ArgumentError, msg = "ArgumentError: the gradient is a NetworkParameters{Float64, (:L1, :L2, :L3), ...}, the parameters are a NetworkParameters{Float32, ...")
zygote   => (history = Vector{Float64}, leaf_eltypes = DataType[Float32])

The seed test (test/pullbacks/symbolic_hnn_pullback.jl:151-159) checks the parameters' type only with data of that same type, so it cannot see this. I did not check whether main fails the same way. Either way the fix belongs in the lines this PR writes: convert the result, not the seed, for example mapstorage(x -> TP.(x .* Δ .* inv_R ./ norm_Y), …), and add a mixed-precision row to the test. Unambiguous.

2 · bug: `CHANGELOG.md:42`, "a bare `NamedTuple` is a `MethodError`"

It is an ErrorException. AbstractNeuralNetworks' untyped fallback (src/pullback.jl:23-29 of 0.9.1) catches what the new ps::NetworkParameters methods exclude (probe_correctness.jl, P2):

P2 NamedTuple ps, pair   => (ErrorException, "Pullback not implemented for input-output pair!")
P2 NamedTuple ps, single => (ErrorException, "Pullback not implemented for single input!")

The PR body repeats the claim. Correct the sentence: a bare NamedTuple reaches AbstractNeuralNetworks' fallback, which raises an ErrorException. Unambiguous.

3 · quality: one evaluation of the Hamiltonian vector field per step is enough

SymbolicPullback's call (src/derivatives/pullback.jl:222-224 of SymbolicNeuralNetworks 0.9.0) computes pullback.loss(model, ps, input, output), which is norm(hvf(X, ps) - Y) / norm(Y). Then it calls pullback.fun(input, output, ps), which evaluates g.loss.hvf(input, ps) again for norm_R (:74). Both compute the same residual. The docstring and the CHANGELOG present the extra evaluation as the price of the fix. It is not: it comes from routing one scalar through upstream's two-field protocol.

Simpler form: a call method that computes the residual once and returns norm_R / norm_Y as the value.

function (pb::SymbolicPullback{<:HNNLoss, <:_HNNParameterGradient})(
        ps::NetworkParameters, model, (X, Y)::Tuple{<:QPTOAT, <:QPTOAT})
    norm_R, norm_Y = norm(pb.loss.hvf(X, ps) - Y), norm(Y)
    T = parameter_eltype(ps)
    norm_R / norm_Y,
    Δ -> mapstorage(x -> T.(x .* Δ .* pinv(norm_R) ./ norm_Y), pb.fun.gradient_function(X, Y, ps))
end

A cleaner option is a pullback type of this package with that one method. It also removes _HNNParameterGradient, its docstring and docs entry, and the fun(input, output, ps)(Δ) protocol that the docstring has to document (half of C7). This needs a design decision. The first form keeps SymbolicPullback(arch) isa SymbolicPullback and adds a method on an upstream type parameterised by own types. The second form changes the type that SymbolicPullback(arch) returns.

Measured in probe_timing_small.jl: two cold processes, BLAS.set_num_threads(1), the test's StandardHamiltonianArchitecture(2, 4, 1), Float64, one step = value plus back(one(value)), @belapsed. The machine was shared, with a load average of 12 to 18. Both variants return bitwise-equal values and gradients at every size.

N	shipped, time / bytes	one evaluation, time / bytes	time ratio, run 1 and run 2
8	5.1 µs / 4048 B	3.8 µs / 2864 B	0.76, 0.95
128	69.2 µs / 32912 B	65.0 µs / 17296 B	0.94, 0.87
1024	593.2 µs / 247888 B	491.7 µs / 124784 B	0.83, 0.89

The single evaluation is faster at every size in both runs, and it halves the allocation from N = 128 up. A larger network, (2, 16, 2), did not finish its symbolic build within 12 minutes, so there is no figure for it.

4 · quality: the underflow guard costs a 7-line comment and a test row, for a regime where the reference fails

:75-81 and the form x .* c .* inv_R ./ norm_Y exist so that the gradient stays non-zero at ‖Y‖ ≈ 1e23 in Float32. The test file itself says that ZygotePullback underflows there (test/pullbacks/symbolic_hnn_pullback.jl:16-18, :69-70). So the symbolic path is built to be more robust than the path it is tested against, on data no one trains on. Simpler: compute s = T(Δ) * pinv(norm_R) / norm_Y once, then x .* s. The product ‖R‖‖Y‖ is still never formed, so the overflow row at 1e20 keeps passing. The underflow row and the comment go. This needs a design decision, because it drops a tested guarantee.

5 · quality: the default loss excludes architectures that already name their loss

NetworkLoss(arch::Union{NeuralNetworkIntegrator, AutoEncoder}, ::Batch{:FeedForward}) = NetworkLoss(arch) sends a StandardHamiltonianArchitecture to the ArgumentError. But NetworkLoss(::HamiltonianArchitecture) = HNNLoss(arch) exists (src/loss/hnn_loss.jl:29), and so does NetworkLoss(::LagrangianNeuralNetwork) (src/loss/lnn_loss.jl:122). From probe_correctness.jl, P4:

P4 NetworkLoss(hnn, Batch(4)) => ArgumentError
P4 NetworkLoss(hnn)           => HNNLoss{…}

test/data_loader/train.jl:147-153 pins the exclusion and gives no reason. Either widen the union to Union{NeuralNetworkIntegrator, AutoEncoder, HamiltonianArchitecture, LagrangianNeuralNetwork}, or state the reason in the docstring. Design decision. In the same block, train.jl writes NetworkLoss(…) bare, while losses.jl:142-143, hnn_loss.jl:29 and lnn_loss.jl:122 write AbstractNeuralNetworks.NetworkLoss. Use one form.

6 · quality: the four-argument `AutoEncoderLoss` drops its target silently

(loss::AutoEncoderLoss)(model, ps, input, output) = loss(model, ps, input). A user who builds a DataLoader with targets for an autoencoder, as test/data_loader/train.jl:166-169 does, trains on reconstruction and gets no signal that the targets went unused. The behaviour is documented, but an argument that is accepted and then ignored is the fragile form. The alternative is no four-argument method, so that the mismatch is a MethodError at the first step, and a DataLoader without output in that test. Design decision.

The routing is also roundabout. The two-argument method (:145-147) calls the four-argument one with input twice, which calls the three-argument one, which calls FeedForwardLoss() with input twice: three hops for one formula. Let :146 call loss(nn.model, params(nn), input). Unambiguous.

7 · quality: `_pullback`

_pullback is now a function (src/data_loader/train.jl:64-65). In the next file it is also the name of the functor's own argument and of the doctest's local variable (src/pullbacks/zygote_pullback.jl:14,17,31,34,49-57). A reader of train! sees _pullback(loss), and in the docstring _pullback(nn.params, nn.model, input), and the two mean different things. The counting test (test/data_loader/train.jl:194-200, :208-210) extends this private function to prove that a call on line 37, outside the loop, runs once. Simpler: pullback = loss isa AbstractPullback ? loss : ZygotePullback(loss), and no build-counting half in the test. The rename alone is unambiguous. Dropping the test half is a judgement.

8 · quality: "decision N" in test comments

Five comments cite "decision 8", "decision 9", "decision 32" and "decision 34". No such document is in the repository: grep -rn -i "decision [0-9]" src test docs scripts CHANGELOG.md KNOWN_ISSUES.md README.md prints exactly these five lines. A reader cannot follow them. Drop the parentheticals, or cite an issue. Unambiguous.

9 · nit: construction builds the symbolic network and the vector field twice

SymbolicPullback(arch) builds SymbolicNeuralNetwork(arch) (:50) and symbolic_hamiltonian_vector_field(nn) (:53). HNNLoss(arch) (:51) builds both again, inside hamiltonian_vector_field(arch) (src/architectures/hamiltonian_neural_network.jl:120-124). Simpler, and unambiguous:

nn = SymbolicNeuralNetwork(arch)
hvf = symbolic_hamiltonian_vector_field(nn)
loss = HNNLoss(SymbolicNeuralNetworks.build_nn_function(hvf, nn.params, nn.input; inplace = false))
soutput = Symbolics.variables(:y, 1:input_dimension(nn.model))
gradient = SymbolicNeuralNetworks.symbolic_parameter_gradient(sum(abs2, hvf - soutput) / 2, nn)

This is for readability only. The warm build time did not change: 0.16 s and 0.15 s for the shipped form, 0.17 s and 0.16 s for this one (probe_timing_small.jl, two runs). The pullback it builds is bitwise equal.

10 · nit: the test file builds the same pullback twelve times

All four testsets of test/pullbacks/symbolic_hnn_pullback.jl use the same StandardHamiltonianArchitecture(2, 4, 1, SigmoidActivation()). Each calls SymbolicPullback(arch) inside its T, N or scale loops: 4 + 4 + 2 + 2 = 12 constructions, plus the HNNLoss(arch) and hamiltonian_vector_field(arch) builds of the Zygote references. At about 0.16 s per warm build, this is about 2 s of the file's 45.7 s on Julia 1 - ubuntu-latest (SymbolicPullback of HNNLoss is the gradient of the batched loss | 228 228 45.7s). The larger cost is clarity: build arch, pb = SymbolicPullback(arch) and zpb = ZygotePullback(pb.loss) once, at file scope. The element type enters through NeuralNetwork(arch, CPU(), T), not through the pullback. Unambiguous.

11 · nit: `src/data_loader/train.jl:35-36`

"One dispatch per call, not one per step: a pullback is cheap to build once and the loop below holds a single concrete closure." The loop holds a pullback struct, not a closure, and a per-step build was never an alternative here. Delete the comment.

12 · nit: CHANGELOG bookkeeping

CHANGELOG.md:54 says "choice (A) of B5's three options … the cost the choice names", and points at an entry that this PR deletes. :58-59 cite commit 3484c6a7. :64 (the ExplicitImports ignore list) is a detail of the test suite. :66 lists the vacant numbers "B1, B2, B3, B4, B6, B8 and B9 (B5), and C9, C12 and C13 (C7)". None of this tells a user what changed for them. Keep the before and after numbers and the migration lines, and drop the rest.

13 · nit: the text of the symbolic pullback

The exported docstring (:8-46) spends most of its length on the private callable and its protocol. The docs page lists _HNNParameterGradient in @docs (docs/src/architectures/hamiltonian_neural_network.md:59). The comment at :79-81 cites "the Y scale 1e23 of the test's underflow row", which ties the source to a test constant. _HNNParameterGradient <: Function (:66) does nothing, because SymbolicPullback{NNLT, FT} puts no bound on FT. Finding 3 removes most of this.

14 · nit: the PR body

Most of the body is process records: critic rounds, "advisor" and "arbitrator" decisions, mutants, a dispatcher. The labels [gml-simplify §1] and [train-design] have no link targets. No text in the body is addressed to a reviewer as an instruction. The "Critic: PASS" and "Pre-PR verification: Clear to open" sections are claims, and findings 1 and 2 contradict them. Fixes #319 and Fixes #343 close both issues on merge, which the body intends.

Verified and fine

ZygotePullback's back(Δ) is a NetworkParameters with keys(ps): the new doctests and test/pullbacks/zygote_pullback.jl check it, and the Doctests job passes.
At matching precision the symbolic gradient equals Zygote's, and P1 gives a maximum relative leaf difference of 5.9e-16 for a Float64 network on Float32 data. CI runs the four testsets green.
A one-argument NetworkLoss(transformer) is a MethodError after the removal (P3), as the CHANGELOG says.
NON_PUBLIC_QUALIFIED in test/quality/aqua.jl holds 9 names, which matches C16's new count.
JuliaFormatter (sciml) passes on the 17 changed Julia files.
git diff --name-status origin/main...origin/pr/344 lists 25 files, and all were read.

Not checked

Whether finding 1 also fails on main. A second environment at the base needs another full precompile on a loaded machine.
Test.detect_ambiguities with the method form of finding 3 added. The argument for it: QPTOAT ⊂ ArrayOrNamedTuple and ps::NetworkParameters make it more specific than both upstream methods in every slot.
The full suite, the docs build and Metal. CI covers them.

…ltype SymbolicPullback(arch) converts each gradient leaf to the parameters' element type, so a Float32 network trains on Float64 data as through ZygotePullback; a test row covers data of the other float type. The pullback test file builds its SymbolicPullback once. AutoEncoderLoss's two-argument method calls the three-argument one directly. The private helper _pullback is renamed _as_pullback, so it no longer shares its name with the ZygotePullback functor argument. Comments drop references to numbered decisions that the repository does not hold. CHANGELOG: a bare NamedTuple reaches ErrorException, not MethodError; process bookkeeping removed. Co-Authored-By: Claude Opus 5.5 <[email protected]>

A call method on SymbolicPullback{<:HNNLoss, <:_HNNParameterGradient} computes the residual once and returns norm(R)/norm(Y) as the value, so the gradient scale no longer needs a second evaluation of hvf. _HNNParameterGradient holds only the generated gradient function and is no longer callable. The constructor builds the symbolic network and its vector field once, and HNNLoss wraps the function built from them. Review findings 3 and 9 of #344. Co-Authored-By: Claude Opus 5.5 <[email protected]>

michakraus

Review of `d51a3a129cdc8891169bb58b0aa8980f79f34e9b` (round 2)

Verdict: comment. This round has no bug-level and no correctness-risk finding. The 11 findings are 3 quality and 8 nit, and they are about text and tests. Scope: the whole diff origin/main...origin/pr/344 (25 files, all read), not only 6291b1bf and d51a3a12.

Focus, as asked: fragile, verbose, inefficient, overcomplicated and unclean code, and where the new code can be drastically simplified. The probes ran cold against a scratch environment that develops this head. Versions: Julia 1.13.1, AbstractNeuralNetworks 0.9.1, NeuralNetworkParameters 0.5.2, SymbolicNeuralNetworks 0.9.0, Zygote 0.7.13. The machine was shared, with a load average of 5 to 8.

Can the new code be drastically simplified? In src/, no further than this head. The one larger step left is to make the HNN pullback a type of this package, and round 1 already named it as a design option. The remaining large reductions are in text and in tests:

src/pullbacks/symbolic_hnn_pullback.jl is 86 lines. 58 of them are docstring or comment (finding 3).
test/pullbacks/symbolic_hnn_pullback.jl repeats one nested leaf loop seven times and builds its Zygote reference 16 times (findings 4 and 6).
test/data_loader/train.jl tests the pullback a second time (finding 5).

CI

All seven required checks pass on d51a3a12 (run 38615001202, headSha checked): Julia min/1 × ubuntu/macOS/windows and Doctests. Documentation, Scripts, both Metal jobs, pre and nightly pass too. Downgrade - ubuntu-latest fails. Its log shows a resolver failure, not a test failure: forcedeps check failed: GPUArraysCore resolved to 0.2.1 but lower bound is 0.2.0, with the same failure for Zygote and ChainRulesCore. The job is continue-on-error: true (.github/workflows/CI.yml:135), and it fails the same way on main at 3484c6a7 (run 38596013154). This PR does not touch Project.toml.

Status of the round-1 findings

round 1	status at `d51a3a12`	evidence
1 mixed precision	fixed	`train!` with a `Float32` network, `Float64` data and `loss = SymbolicPullback(arch)` returns a `Vector{Float64}`. Testset 5 of the pullback file covers it
2 `MethodError` claim	fixed in `CHANGELOG.md:42`, still in the PR body	finding 2
3 two evaluations	fixed	`src/pullbacks/symbolic_hnn_pullback.jl:71` is the only `hvf` call. `Test.detect_ambiguities(GeometricMachineLearning, SymbolicNeuralNetworks)` gives 5 pairs, and none of them touches a changed file
4, 5 (union), 6 (arity)	standing decisions	not raised again
5, second half	open	finding 9
6, routing	fixed	`src/loss/losses.jl:145-157`
7	rename fixed. The author kept the build-counting test	not raised again
8	fixed	`grep -rn -i "decision [0-9]" src test docs scripts CHANGELOG.md KNOWN_ISSUES.md README.md` is empty
9, 11	fixed	read
10	partly fixed	finding 4
12	partly fixed	finding 10
13	partly fixed	finding 3
14	open, and now with false statements	finding 2

Findings

"Design" means that the fix changes a public type or an exported signature, or drops a tested guarantee.

#	severity	file:line	claim	design
1	nit	`test/pullbacks/symbolic_hnn_pullback.jl:125-126`, `:151-152`	the comments say that the seed is converted. Since `6291b1bf` the gradient leaf is converted	no
2	quality	PR body	four statements are false at this head	no
3	quality	`src/pullbacks/symbolic_hnn_pullback.jl:1-42`, `:57-62`, `:73-79`; `docs/src/architectures/hamiltonian_neural_network.md:59`	58 of 86 lines are text, and the same three facts are written in four places	no
4	nit	`test/pullbacks/symbolic_hnn_pullback.jl:40,56,91,109,135,175`	the Zygote reference and `hvf` are built 16 times inside the loops	no
5	nit	`test/data_loader/train.jl:116`, `:121-129`	the `train!` testset tests the pullback a second time	no
6	quality	`test/pullbacks/symbolic_hnn_pullback.jl` (7×), `test/data_loader/train.jl` (2×), `test/pullbacks/zygote_pullback.jl` (1×)	one nested leaf loop is written 10 times	no
7	nit	`src/pullbacks/zygote_pullback.jl:8-40`	two doctests are the same except for the last line, and they import exported names	no
8	nit	`test/loss/losses.jl:71-72`	`@test_throws "Functor not defined"` matches the text of an upstream message	no
9	nit	`src/data_loader/train.jl:68-73` vs `src/loss/losses.jl:142-143`	the new `NetworkLoss` methods use two spellings	no
10	nit	`CHANGELOG.md:64`; `src/loss/losses.jl:142`	a new public method has no entry, and one entry repeats `KNOWN_ISSUES.md`	no
11	nit	`src/pullbacks/symbolic_hnn_pullback.jl:70`; adjacent `src/loss/hnn_loss.jl:31-41`	`Tuple{<:QPTOAT, <:QPTOAT}` accepts `(q, p)` data that fails deep inside	yes, for the `HNNLoss` part

1 · nit: stale comments about the seed

:125-126 reads: "a seed of the parameters' element type comes back in that element type whatever seed Δ is given". :151-152 reads: "a seed of the other float type is converted to the parameters'". The code does not convert the seed. src/pullbacks/symbolic_hnn_pullback.jl:84 computes TP.(x .* Δ .* inv_R ./ norm_Y), so Δ promotes and the leaf is then converted. Proposed text:

:125-126: "The gradient is linear in the seed, as ZygotePullback's is, and has the parameters' element type whatever the seed's."
:151-152: "a seed of the other float type gives the same gradient, in the parameters' element type".

2 · quality: the PR body says four things that are false at this head

A squash merge puts the body into the history. These four statements are false:

"_pullback(loss) is called once per train! call". The function is now _as_pullback (src/data_loader/train.jl:36, :63-64).
"a bare NamedTuple is a MethodError". It is an ErrorException, as CHANGELOG.md:42 now says.
"one evaluation of the Hamiltonian vector field more than the loss value alone". Since d51a3a12 the call evaluates it once (:71).
"The two-field constructor SymbolicPullback(loss, fun) and the call protocol fun(input, output, ps)(Δ) are documented under Implementation". _HNNParameterGradient cannot be called (:63-65), and the docstring documents no such protocol.

The process records of round-1 finding 14 are also still in the body: critic rounds, arbitrator decisions, mutants, and a "Critic: PASS" section at 80fc5ba3. No text in the body tells a reviewer what to do. Proposed fix: rewrite the body as a summary of the change at this head. Fixes #319 and Fixes #343 close both issues on merge, which the body intends.

3 · quality: the symbolic pullback file is two-thirds text

These parts of the 86 lines are text: docstring :1-42 (42 lines), the _HNNParameterGradient docstring :57-62 (6), and comments :46, :67-68 and :73-79 (10). Each of these three facts is written in the docstring (:26-29, :37-41), in the comment :73-79, in CHANGELOG.md:54 and in the test comments (test/…:16-18, :61-64):

pinv gives a zero gradient at a zero residual.
The product ‖R‖‖Y‖ overflows, and its inverse underflows.
The leaf is converted to the parameters' element type.

:35 says "The construction below", which has no meaning in the rendered docs. _HNNParameterGradient is a private one-field tag type, but it has a docstring and an @docs entry on a public page (docs/src/architectures/hamiltonian_neural_network.md:59).

Proposed fix. The guard stays, by your decision on round-1 finding 4. Only the text changes.

"""
    SymbolicPullback(arch::HamiltonianArchitecture)

Make a `SymbolicPullback` of [`HNNLoss`](@ref) for a [`HamiltonianArchitecture`](@ref).

Its value is `HNNLoss` on the batch, ``\\ell = \\|R\\|/\\|Y\\|`` with ``R = X_H(X) - Y``. Its gradient is
``\\Delta \\sum_i \\nabla_\\theta \\tfrac12\\|r_i\\|^2 / (\\|R\\|\\,\\|Y\\|)``: the generated per-sample
gradient of the surrogate ``\\tfrac12\\|r_i\\|^2``, summed over the batch and scaled by the two norms
of the value's residual. The gradient has the parameters' element type, and a zero residual gives a
zero gradient.

`SymbolicNeuralNetworks.SymbolicPullback(nn, loss)` cannot be used: it sizes the target of the loss
with `output_dimension(nn.model)`, the scalar Hamiltonian, but `HNNLoss` compares against the vector
field, which has the dimension of the input.
"""

Replace the comment :73-79 with: "pinv: a zero residual gives the zero gradient, not NaN. Each norm divides each entry on its own, because ‖R‖‖Y‖ overflows, and its inverse underflows, where neither norm does."
Replace the _HNNParameterGradient docstring with a one-line comment: "the fun of the HNN pullback; it holds the generated gradient and selects the call method below".
Delete docs/src/architectures/hamiltonian_neural_network.md:59.

This leaves about 20 lines of text for about 28 lines of code.

4 · nit: the Zygote reference is built 16 times (the open part of round-1 finding 10)

SymbolicPullback(arch) is now built once (:24). But ZygotePullback(HNNLoss(arch)) and GeometricMachineLearning.hamiltonian_vector_field(arch) are still built inside the T, N and row loops: 8 times in testset 1 and 2 times in each of the other four testsets. A warm build takes 0.011 s (probe: HNNLoss(arch) 0.011 s, hamiltonian_vector_field(arch) 0.011 s, SymbolicPullback(arch) 0.154 s), so the cost is clarity, not time. Proposed fix, at file scope:

const reference = ZygotePullback(HNNLoss(arch))
const hvf = GeometricMachineLearning.hamiltonian_vector_field(arch)

Do not use ZygotePullback(pullback.loss) here. A separate HNNLoss(arch) keeps the reference independent of the code that the file tests.

5 · nit: the `train!` testset tests the pullback a second time

test/data_loader/train.jl:121-129 compares the symbolic value and gradient with Zygote's. :116 asserts the leaf element type. test/pullbacks/symbolic_hnn_pullback.jl:30-59, :156 and :181 already assert both, for the same architecture and for both float types. The train! testset needs only the step identity layer ≈ layer₀ .- η .* gradient, layer != layer₀ and history == [value] (:111-120). Proposed fix: delete :116 and :121-129, and remove HNNLoss from the import on :2, because :121 is its only use.

6 · quality: one leaf loop is written 10 times

Ten loops have the same shape, for (name, leaf) in pairs(g); for k in keys(leaf); @test …(leaf[k], zg[name][k]); end; end: seven in the pullback file (:48-53, :82-88, :94-98, :117-122, :142-149, :154-159, :179-185), two in test/data_loader/train.jl (:114-120, :124-129) and one in test/pullbacks/zygote_pullback.jl (:48-54). A helper in test/helpers/ keeps the report for each leaf and halves each block:

# Call `f` on each leaf of `g` and on the leaf of the same name in each of `refs`.
function foreach_leaf(f, g, refs...)
    for name in keys(g), k in keys(g[name])
        f(g[name][k], (r[name][k] for r in refs)...)
    end
end

foreach_leaf(g, zg) do leaf, ref
    @test eltype(leaf) === T
    @test leaf ≈ ref rtol = T(√eps(T))
end

first(NeuralNetworkParameters.flatten(g)) ≈ first(flatten(zg)) is shorter still. But it compares the norm of the whole vector, not each leaf, so it is a weaker check, and I do not propose it.

7 · nit: two doctests that are almost the same

src/pullbacks/zygote_pullback.jl:8-23 and :25-40 have the same eight setup lines and differ only in the last expression. Both have using GeometricMachineLearning: AutoEncoderLoss, ZygotePullback, but both names are exported (probe: Base.isexported(GeometricMachineLearning, :AutoEncoderLoss) == true, and the same for :ZygotePullback). Proposed fix: one doctest.

```jldoctest
using GeometricMachineLearning

pb = ZygotePullback(AutoEncoderLoss())
nn = NeuralNetwork(Chain(Dense(10, 2, tanh), Dense(2, 10, tanh)))
value, back = pb(nn.params, nn.model, rand(10))
g = back(1)
(g isa NetworkParameters, keys(g))

# output

(true, (:L1, :L2))
```

test/integration/docstrings/utilities.jl:18-25 copies the doctest, so change it in the same commit.

8 · nit: the test matches the text of an upstream message

test/loss/losses.jl:71-72 has @test_throws "Functor not defined" tr_loss(...). The string is the incidental text of AbstractNeuralNetworks' fallback error. A change to that text breaks the test, but the behaviour stays the same. Proposed fix: @test_throws ErrorException.

9 · nit: two spellings of `NetworkLoss` in the new methods (the open part of round-1 finding 5)

Your decision covers the union. It does not cover the spelling. src/loss/losses.jl:142-143 (NetworkLoss(::NeuralNetworkIntegrator) is new in this PR), hnn_loss.jl:29 and lnn_loss.jl:122 define AbstractNeuralNetworks.NetworkLoss(...). src/data_loader/train.jl:68, :69 and :73 define a bare NetworkLoss(...), which works through import AbstractNeuralNetworks: …, NetworkLoss (src/GeometricMachineLearning.jl:83). Proposed fix: qualify the three definitions in train.jl.

10 · nit: CHANGELOG

src/loss/losses.jl:142 adds the public method NetworkLoss(::NeuralNetworkIntegrator) = FeedForwardLoss(). No entry names it. Add one under Added.
:64 repeats KNOWN_ISSUES.md: "C7 is rewritten rather than closed …" and "K14 records …". That is the remainder of round-1 finding 12. Keep "closes #319 and #343" and drop the rest. KNOWN_ISSUES.md already holds C7 and K14.

11 · nit: the signature accepts `(q, p)` data that cannot work

Tuple{<:QPTOAT, <:QPTOAT} at :70 accepts NamedTuple data. The call then fails inside pb.loss.hvf. Probe, with (q = randn(1, 5), p = randn(1, 5)) as input and as output:

A symbolic QPT data  => (MethodError, "MethodError: no method matching ndims(::@NamedTuple{q::Matrix{Float64}, p::Matrix{Float64}})…")
A HNNLoss QPT data   => (MethodError, same)
A zygote QPT data    => (MethodError, same)

This defect is older than the PR: HNNLoss's two functors (src/loss/hnn_loss.jl:31-41) make the same promise. If only the new method is narrowed, (q, p) data goes to the upstream method and fails in the same way. So the fix has an effect only together with the HNNLoss part. Proposed fix: Tuple{<:AbstractArray, <:AbstractArray} at :70, and AbstractArray on HNNLoss's two functors, in this change. The HNNLoss part changes an exported functor signature and needs a decision, although no call that works today changes.

Verified and fine

Inference: Base.return_types of pullback(ps, model, (X, Y)) is a concrete Tuple{T, closure} for T = Float32 and Float64, and back(Δ) infers a concrete NetworkParameters{T, …}.
One sample as a Vector input: the symbolic value equals Zygote's, and the largest relative leaf difference is 1.7e-15.
Mixed precision through train! (round-1 finding 1): Vector{Float64}, with Float32 leaves.
Removed API: grep -rn -E "pullback ?=|_processing|_get_contents|_default_loss" docs/src scripts src test README.md ext finds only the local variables _pullback and pullback, and the @test_throws MethodError test of the removed keyword.
JuliaFormatter (sciml) passes on all 20 changed Julia files.
test-layout.jl --check on the worktree reports no violation.

Not checked

The full suite, the docs build and Metal. CI covers them at this head.
The time of the one-evaluation call at this head. Round 1 measured the same form before it was merged. I did not measure it again.
Whether each core file still runs in less than 60 s cold.

- SymbolicPullback(::HamiltonianArchitecture): a compact docstring, a two-line comment on pinv and the per-norm division, and a one-line comment in place of the private _HNNParameterGradient docstring, whose @docs entry goes too. - ZygotePullback: one doctest instead of two, without the import of exported names; the docstring test copy follows it. - The three NetworkLoss methods in src/data_loader/train.jl are spelled AbstractNeuralNetworks.NetworkLoss, as in src/loss/. - Tests: a foreach_leaf helper in test/helpers/leaves.jl replaces ten nested leaf loops; the symbolic pullback test builds its Zygote reference and vector field once; the train! test no longer repeats the pullback's own Zygote comparison; the stale seed comments are corrected; the TransformerLoss refusal is tested by exception type. - CHANGELOG: an Added entry for NetworkLoss(::NeuralNetworkIntegrator), the closing entry cut to the issue closures, and the migration sentence of the transformer entry names only the transformer calls. Co-Authored-By: Claude Opus 5.5 <[email protected]>

michakraus added 23 commits October 14, 2026 03:05

Docstring mirror: bind the loss value to _, which is unused

8256f46

The train! docstring carries no history of the pullback keyword

f50e233

The CHANGELOG's migration note for the removed keyword names 0.8; the docstring states the present signature.

The module header counts the two names it takes from NeuralNetworkPar…

69cccc2

…ameters The pullback part adds `parameter_eltype` to the import, so `mapstorage` is no longer the one thing that comes from there rather than being written again here.

The test says the scale divides at both batch sizes

541d65e

The scale `1/(‖R‖‖Y‖)` is not one at `N = 1`, where the batch is one sample and the sum is a single per-sample gradient; the batch of five is where the per-sample sum's gradient and the batch loss's gradient differ.

C7 says which private name the code does not use, in the present tense

0cb4e21

michakraus commented Oct 14, 2026

View reviewed changes

michakraus and others added 2 commits October 14, 2026 19:58

michakraus commented Oct 14, 2026

View reviewed changes

Navigation Menu

Repository navigation

Losses and pullbacks: one default loss, one pullback shape, and the symbolic HNN gradient (B5) - #344

Losses and pullbacks: one default loss, one pullback shape, and the symbolic HNN gradient (B5)#344
michakraus wants to merge 26 commits into
mainfrom
simplify/part-m1b

michakraus commented Oct 14, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Oct 14, 2026 •

edited

Loading

Uh oh!

michakraus left a comment

Uh oh!

michakraus left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

michakraus commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

codecov Bot commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michakraus left a comment

Choose a reason for hiding this comment

Review of 0cb4e21fffa265d6dc4dbc87f32ceb140b9f1dc8

CI

Findings

1 · bug: the symbolic pullback breaks train! for a Float32 network on Float64 data

2 · bug: CHANGELOG.md:42, "a bare NamedTuple is a MethodError"

3 · quality: one evaluation of the Hamiltonian vector field per step is enough

4 · quality: the underflow guard costs a 7-line comment and a test row, for a regime where the reference fails

5 · quality: the default loss excludes architectures that already name their loss

6 · quality: the four-argument AutoEncoderLoss drops its target silently

7 · quality: _pullback

8 · quality: "decision N" in test comments

9 · nit: construction builds the symbolic network and the vector field twice

10 · nit: the test file builds the same pullback twelve times

11 · nit: src/data_loader/train.jl:35-36

12 · nit: CHANGELOG bookkeeping

13 · nit: the text of the symbolic pullback

14 · nit: the PR body

Verified and fine

Not checked

Uh oh!

michakraus left a comment

Choose a reason for hiding this comment

Review of d51a3a129cdc8891169bb58b0aa8980f79f34e9b (round 2)

CI

Status of the round-1 findings

Findings

1 · nit: stale comments about the seed

2 · quality: the PR body says four things that are false at this head

3 · quality: the symbolic pullback file is two-thirds text

4 · nit: the Zygote reference is built 16 times (the open part of round-1 finding 10)

5 · nit: the train! testset tests the pullback a second time

6 · quality: one leaf loop is written 10 times

7 · nit: two doctests that are almost the same

8 · nit: the test matches the text of an upstream message

9 · nit: two spellings of NetworkLoss in the new methods (the open part of round-1 finding 5)

10 · nit: CHANGELOG

11 · nit: the signature accepts (q, p) data that cannot work

Verified and fine

Not checked

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

michakraus commented Oct 14, 2026 •

edited

Loading

codecov Bot commented Oct 14, 2026 •

edited

Loading

Review of `0cb4e21fffa265d6dc4dbc87f32ceb140b9f1dc8`

1 · bug: the symbolic pullback breaks `train!` for a `Float32` network on `Float64` data

2 · bug: `CHANGELOG.md:42`, "a bare `NamedTuple` is a `MethodError`"

6 · quality: the four-argument `AutoEncoderLoss` drops its target silently

7 · quality: `_pullback`

11 · nit: `src/data_loader/train.jl:35-36`

Review of `d51a3a129cdc8891169bb58b0aa8980f79f34e9b` (round 2)

5 · nit: the `train!` testset tests the pullback a second time

9 · nit: two spellings of `NetworkLoss` in the new methods (the open part of round-1 finding 5)

11 · nit: the signature accepts `(q, p)` data that cannot work