Finish the separation from GeometricOptimizers - #241
Conversation
Eleven types were defined in both packages -- `Manifold`, `StiefelManifold`,
`GrassmannManifold`, `SkewSymMatrix`, `SymmetricMatrix`, `AbstractTriangular`,
`LowerTriangular`, `UpperTriangular`, `AbstractLieAlgHorMatrix`,
`StiefelLieAlgHorMatrix`, `GrassmannLieAlgHorMatrix`, `StiefelProjection` -- as
near-verbatim copies. Julia saw them as distinct types, so none of
GeometricOptimizers' generic machinery dispatched on GML's: `geodesic(::Manifold,
::AbstractMatrix)`, `cayley`, `global_rep`, `apply_section!` and `update_section!`
all had to be written again per manifold, and `src/optimizers/go_bridges.jl`
existed to reconnect the two hierarchies with about thirty more methods.
They are `import`ed now and the copies are gone: 2595 deletions against 202
insertions. `import` and not `const X = GeometricOptimizers.X` -- with `using
GeometricOptimizers: X`, every constructor GML adds to an imported type warns
("Constructor for type X was extended without explicit qualification"), and an
`import` is also where a reader looks to find out where a name comes from.
This is issue B2, and it needed GeometricOptimizers#50 first: `SymmetricMatrix`
and the triangular types had no `similar`, `fill!` or elementwise primitives
there, so they could not have been optimizer parameters.
## Three bugs that the duplication was hiding
Each was live in GML's copy and already fixed in GeometricOptimizers', so
importing fixes them. All three were reproduced before and after.
- **`rand(3,3) + SkewSymMatrix(...)` was a `StackOverflowError.`**
`Base.:+(B::AbstractMatrix, A::SkewSymMatrix) = B + A` called itself. Upstream
has `= A + B`.
- **`parent(::StiefelLieAlgHorMatrix)` returned an unbound `B`.** It read
`(A, B)` where it meant `(A.A, A.B)`; upstream's
`vec(::AbstractLieAlgHorMatrix)` is built on `parent`, so the two disagreed.
- **A decaying step size was read one iteration early.**
`optimization_step!` read the step size *before* incrementing `opt.iterations`,
so a run's first step was `α(0) = η₁` where the pre-0.5 code incremented first
and took `α(1) = γη₁`. Every step of a run sat one place early in the schedule.
Upstream's `test/adam_optimizer_with_decay.jl` asserts the behaviour this
restores.
## One `AdamOptimizerWithDecay` (issue B1)
Both packages exported the name, so `using GeometricMachineLearning,
GeometricOptimizers` was an `UndefVarError` on it. They were the same algorithm --
Adam's direction with a geometrically decaying learning rate, the same
`γ = exp(log(η₂/η₁)/n)`, values identical to the last bit -- differing only in
packaging: a `struct <: OptimizerMethod` carrying both halves here, an
`(algorithm, linesearch)` pairing upstream, where the step size belongs to a
`LinesearchMethod`.
GML's struct is deleted and upstream's name imported. `Optimizer` takes the
pairing by splatting, as upstream's does:
Optimizer(nn; AdamOptimizerWithDecay(n_epochs, Float32)...)
`step_size` accepts a `DecayingStatic` as well as a number, and
`_current_step_size` asks the schedule. The `AdamOptimizerWithDecay` *methods* of
`_is_go_native_method`, `_adapt_method_to_T`, `_default_step_size` and `_current_step_size`
go with the struct -- the functions themselves stay, they still dispatch on the upstream
methods -- and so does a `_euclidean_update!` that was a verbatim copy of the `Adam` one.
`Optimizer` is now the only name both packages export -- twelve did before -- and
it stays until the parameter-tree traversal moves upstream too. See C1.
## What stayed
`src/arrays/gml_extensions.jl` holds the methods that are genuinely about neural
networks rather than geometry: `add!` (AbstractNeuralNetworks') and
`networkbackend` for the imported types. `PoissonTensor` stays where it was. The
tensor kernels and their `rrule`s keep dispatching on the imported types, which
is what they always did -- only the module the types come from changed.
`update!` and `solve!` stopped being exported. Neither was GML's: `update!` had
zero methods here and the export only shadowed upstream's, which is the one that
has methods for the caches; `solve!` was a second generic function of the same
name. Both were export collisions of exactly B1's kind.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-three files go. They fall into three groups.
**Twelve duplicated upstream's suite.** `test/arrays/` and `test/manifolds/`
tested the eleven types the previous commit stopped defining, so they were a
second copy of `test/special_matrices/`, `test/lie_algebras/`, `test/manifolds/`
and `test/global_sections/` over there -- against the same types, now that the
types are the same. Each was diffed against its upstream counterpart first and
what upstream did not already assert was ported there (GeometricOptimizers#50);
`test/arrays/poisson_tensor.jl` stays, because `PoissonTensor` is GML's.
Folding them in found four defects in upstream's suite, including three functions
defined and never called and a `stiefel_global_section` that built a
`GrassmannManifold` -- so the Stiefel global section had no test at all. Fixed
there.
**Eight were unreachable from `runtests.jl` and could not have run.**
`optimizers/lie_alg_lifts.jl` and `optimizers/hor_lift.jl` include
`../src/arrays/skew_sym.jl` and `../src/optimizers/householder.jl`, paths that
stopped existing long before this branch. Others `using Lux`, which is not a
dependency. Nothing ran them, so nothing reported the rot.
**Three tested behaviour that is now upstream's**, including
`test/docstrings/manifolds.jl`, whose docstrings moved with the documentation.
## What stayed, and why
`optimizer_convergence_tests/{svd,psd}_optim.jl`,
`optimizers/{gradient,momentum}_optimizer.jl`,
`optimizers/structured_array_parameters.jl`, `optimizers/utils/optimization_step.jl`,
`layers/manifold_layers.jl` and `transformer_related/*` all drive
`Chain`/`NeuralNetwork`/`optimization_step!`. They look like upstream's
`test/optimizer_convergence/svd_optim.jl` and are not: that one drives `solve!`
against an objective, these train a network with gradients from Zygote. Different
code path, both worth having.
## New coverage
`adam_with_learning_rate_decay.jl` gains two tests for the previous commit's
fixes: that the pairing works on `StiefelManifold` weights (GML's own
`AdamOptimizerWithDecay` was a distinct `OptimizerMethod` that had to be routed
onto Adam's cache explicitly, and without the routing manifold weights fell
through to the Euclidean state, whose zero element is not a manifold point), and
that the schedule is walked from `t = 1`, asserted both through
`_current_step_size` and through `optimization_step!`.
## Progress bars
A test that trains passes `show_progress = false` now -- nine call sites. The
`Optimizer` functor defaults it to `true`, which is right at a REPL and is noise
in a suite: the 2048-epoch run in `adam_with_learning_rate_decay.jl` alone emitted
a few hundred progress lines, which is what a failure would have had to be found
in. The default is unchanged, so interactive use is unaffected, and `runtests.jl`
records the convention so it does not creep back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirteen pages leave. They documented types that live in GeometricOptimizers and,
after the first commit of this branch, are not even defined here any more.
Manifolds (all 7 pages) general topology through homogeneous spaces
arrays/skew_symmetric_matrix.md the structured matrix types
arrays/global_tangent_spaces.md 𝔤^hor, global sections
optimizers/ (all 4 pages) the framework, retractions, parallel
transport, the methods
They land upstream as a `Manifolds` section, `special_matrices.md`,
`global_tangent_spaces.md`, `parallel_transport.md` and `optimizer_methods.md`;
`optimizer_framework.md`'s theory merges into `manifold_optimizers.md` and the
retraction theory into `retractions.md`. See GeometricOptimizers#50.
## Three pages split rather than moved
- **`optimizer_framework.md`** documented GML's own `Optimizer`,
`optimize_for_one_epoch!` and `optimization_step!`. The theory goes; a new
`docs/src/optimizers/optimizer.md` keeps those three and says what this package
adds to the framework -- walking the parameter tree of a `NeuralNetwork` and
driving it from a data loader.
- **`arrays/skew_symmetric_matrix.md`**'s *Parallel Computation* section documented
`tensor_mat_mul`/`mat_tensor_mul`, which are GML's. Folded into
`arrays/tensors.md`.
- **`arrays/tensors.md`** and **`pullbacks/computation_of_pullbacks.md`** stay
whole: GML's kernels and GML's AD.
## Cross-references
Thirty-six references from pages that stayed into pages that left, and they stay
references: `DocumenterInterLinks` joins `docs/Project.toml` and `make.jl`. This
also closes **C3**, which asked for exactly this -- `𝔄`, `cayley` and `update!`
had been downgraded to plain code spans as a stopgap.
The inventory is read from a **committed file**,
`docs/inventories/GeometricOptimizers.toml`, and not from a URL. The anchors this
needs only appear in upstream's published `objects.inv` once 0.4.0's documentation
deploys, and a documentation build that cannot run until an unrelated deploy has
happened is a build that will be broken again later for the same reason. The
comment above `links = InterLinks(...)` has the one-line command that regenerates
it.
Two things about `@extref` targets that cost a build each, recorded so the next
person does not pay them again: the target is the inventory's **slug**
(`The-Grassmann-Manifold`, not `"The Grassmann Manifold"`), and for a binding it is
**module-qualified** (`GeometricOptimizers.SymmetricMatrix`). A bare `[Title](@ref)`
with no explicit target is a third form and needs converting too.
## The book loses a part and a chapter
`_html_pages` drops `Manifolds` and `Optimizer`; `Special Arrays and AD` keeps two
of its four pages. In `_latex_pages` the whole `Manifolds` chapter and the whole
`Optimizer` part go, so `docstring_index.md` is rewritten to match and
`abstract.md`, `introduction.md` and `outlook.md` no longer promise chapters that
are not there -- they point at upstream instead.
`_optimizers` is now a single page and goes into `_latex_pages` as a
`Pair{String, String}` rather than as a one-element vector: `Dict(_latex_pages)`
infers its value type from these entries, and one vector-of-strings among the
vector-of-pairs chapters makes that inference fail outright.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AdamOptimizerWithDecay` returns an `(algorithm, linesearch)` pairing now rather
than an `OptimizerMethod`, so the three scripts that used it splat it into
`Optimizer` instead of passing it positionally. The bindings are renamed from
`…_method` to `…_pairing` to say which of the two it is -- a pairing passed where a
method is expected is otherwise a `MethodError` several frames away from the line
that caused it.
`scripts/Project.toml` gains `[sources] GeometricMachineLearning = {path = ".."}`,
replacing a comment that said the path should stay out of the file "so that it
works from any clone". That reasoning predates `[sources]`: the path is relative to
the file, so it does work from any clone, and it removes the one-off
`Pkg.develop(path = "..")` the comment asked for. This edit was in the working tree
before the branch and is kept deliberately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The example predated the move of the optimizer machinery to
GeometricOptimizers and no longer ran:
- `Optimizer(AdamOptimizer(), g_nn)` becomes
`Optimizer(Adam(type), g_nn; step_size = 1e-3)`. The method comes first and the
learning rate is no longer part of it. `Adam` is constructed with the element
type, so it reuses the `type` binding defined a few lines above -- which also
makes the point that the example is type-generic.
- `Iterate_Sympnet` is not defined in the package
(`isdefined(GeometricMachineLearning, :Iterate_Sympnet) == false`), so the line
raised an `UndefVarError` as written. `iterate` is what the SympNet tutorial
uses.
A paragraph says where the optimizer methods now come from, since the README is
where a reader meets them first and `Adam` no longer being a GML type is otherwise
unexplained.
This is the content of #237, which was opened against the pre-CairoMakie README and
whose plotting hunk #238 has since superseded. Closes #237.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `[sources]` entries that pointed `GeometricOptimizers` at a sibling checkout
are gone from `Project.toml` and `docs/Project.toml`, so both environments resolve
it from the General registry -- `registries = "General"`, no `path` key. 0.4.0 is
where the interface GML now imports became public API, so `[compat]` is `"0.4"` and
GML does not load against 0.3.
Removing them is not `Pkg.free`, which fails here with "could not find source path
for package GeometricOptimizers", and not `Pkg.resolve` either -- that infers
`[sources]` from the manifest and writes the entry straight back. Delete the two
entries, delete the (gitignored) manifests, `Pkg.instantiate`.
`docs/inventories/GeometricOptimizers.toml` is regenerated from the *deployed*
0.4.0 documentation rather than from a local build. It had drifted by one heading --
a trailing period removed upstream after the file was first generated -- which
nothing here linked to, but a stale inventory is a dead link waiting for someone to
add the reference.
The changelog gains two entries under *Open Issues* that were being tracked
outside it:
C10 ten exported names are undefined, measured with
`[n for n in names(GML) if !isdefined(GML, n)]`. This release removed three
of the thirteen; the rest each need a decision, and upstream's
`test/exports.jl` shows what closes the class.
C11 41 test files are unreachable from `runtests.jl`. Not one problem but three
-- GPU tests, performance probes, and the `train!` suite that B6 says is
broken -- which is why the entry asks for a decision per group rather than a
deletion.
The `[Unreleased]` heading stays until the release is tagged. The version is
`0.5.0`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ed30aee8` was about bringing the existing scripts to the new `AdamOptimizerWithDecay`, but it also added four files that were untracked in the working tree at the time: `enzyme.jl`, `zygote.jl`, `sae_script2.jl` and `harmonic_oscillator_sympnet.jl`. Nothing in the commit message, the PR description or the changelog mentions them, and they do not look like they were meant to be committed -- `enzyme.jl` needs `Enzyme`, which is not in `scripts/Project.toml`, and `sae_script2.jl` reads `../docs/src/tutorials/*.jld2` by relative path and writes a thousand PDFs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The PDF workflow runs on every pull request and this branch breaks it twice, in ways none of the three gates the branch was checked against would notice: `Pkg.test()`, `check_references.jl` and `docs/make.jl` all leave the book alone. `copy_png_files` ran `find` over `build/manifolds` and `build/optimizers/manifold_related`. Documenter only creates a `build/` subdirectory for a page tree that still has pages, so both went away with the chapters, and `find` exits non-zero on a missing root -- which aborts the recipe and fails the "Some sed magic" step. The two dead roots are gone and the two that remain are guarded, so the next chapter to move out does not break this again. The title page pulled in `parallel_transport_naked.png`, which was generated by a `@example` block in the `parallel_transport.md` that moved upstream. `*.png` is gitignored, so nothing produces the file and the title page came up with a missing graphic. `tikz/tangent_vector_light.png` is built by `make latex -C docs/src/tikz` from a source that is still here and shows the same subject; swap it in if you would rather have different cover art. Six entries in `adjust_image_size.jl` resized figures from those same deleted pages. They matched nothing, which is silent -- `adjust_image_size` no-ops when the pattern is absent -- so they are removed rather than repointed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Optimizer` grew two entry points for the same quantity, and they did not accept
the same things. `linesearch = Static(α)` was a fixed learning rate, but
`step_size = Static(α)` -- the same request through the older keyword -- fell off
the end of `_optimizer_step_size` with `MethodError: no method matching
_optimizer_step_size(::Static{Float64})`, leaking an internal helper's name. The
carefully written `ArgumentError` explaining that a training loop has no objective
for a line search to search along was only reachable from one of the two. Anything
that is not a plain number now goes through that one funnel.
`test/.../multi_head_attention_stiefel_optim_cache.jl` still said a blanket `using`
would be ambiguous "since GeometricMachineLearning re-exports its own versions of
them". That was the reason this branch removes: the shared types are one object
under two names now, and `Optimizer` is the only name left that resolves to two
different things.
The note on the phantom `Symplectic*` exports pointed at `test/exports.jl`, which
does not exist here -- it is GeometricOptimizers'. Ten exported names are still
undefined in GML, which is C10, so the note now says which package the file is in
and what it would take to close the class here.
The GPU snippet in the symplectic autoencoder tutorial paired a `Float32` network
with `AdamOptimizerWithDecay(integrator_train_epochs)`, which is `Float64` now that
the type is positional rather than taken from `η₁`. `OptimizerCache` rejects that
pairing, so the snippet could not have run as printed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
michakraus
left a comment
There was a problem hiding this comment.
Review
I read the whole branch and verified the behavioural claims against the registered GeometricOptimizers 0.4.0 in a scratch worktree rather than taking the PR description on trust. The description holds up unusually well — I checked the load-bearing claims and they are accurate:
- All twelve shared types are genuinely the same objects:
GeometricMachineLearning.SkewSymMatrix === GeometricOptimizers.SkewSymMatrixand so on for all of them, 0 identity mismatches. - Of 36 shared exported names,
Optimizeris the only one that still resolves to two different objects — exactly as claimed, and the rest of B1 is closed. - The undefined-export count is exactly 10, matching the C10 figure.
- The increment-before-read reorder in
optimization_step!is right: upstream'sstep_size(::DecayingStatic, t) = γ^t·η₁, sot = 1on the first step givesα(1) = γη₁, which is what the pre-0.5 code took. - Every name imported or reached as
GeometricOptimizers.Xexists in 0.4.0. Pkg.test()passes (exit 0, 0 failures) anddocs/check_references.jlreports 0 unresolved, both against the registered 0.4.0.test/arrays/triangular.jlis a real catch, twice over: the loop of bare≈expressions asserted nothing, andAᵤ = rand(LowerTriangular…)meant the "upper" case was never tested.
Four things needed fixing, and I've pushed them as three commits. The interesting part is where they were: three of the four sit in the LaTeX book pipeline, which none of the three gates this branch was verified against touches — Pkg.test(), check_references.jl and docs/make.jl all leave docs/Makefile and preamble.tex alone. The PDF workflow runs on every pull request, so CI would have caught them, but only after this landed.
Fixed
1. docs/Makefile:124 — the PDF workflow aborts. copy_png_files ran find over build/manifolds and build/optimizers/manifold_related. Documenter only creates a build/ subdirectory for a page tree that still has pages, so both vanished with the moved chapters — and find exits 1 on a missing root, which aborts the make recipe:
find: build/manifolds: No such file or directory
make: *** [copy_png_files] Error 1
The two dead roots are gone, and the two that remain are guarded with [ -d … ] so the next chapter to move out does not reintroduce this.
2. docs/src/assets/preamble.tex:32 — a missing graphic on the book's title page. \includegraphics{parallel_transport_naked.png} was generated by an @example block at optimizers/manifold_related/parallel_transport.md:171, one of the deleted pages. *.png is gitignored, so the file is not committed and nothing produces it any more. I swapped in tikz/tangent_vector_light.png — built by make latex -C docs/src/tikz from a source that is still here, and the same subject, a tangent vector to a manifold. This is cover art on your dissertation, so it is your call; change it if you'd rather have something else.
3. scripts/{enzyme,zygote,sae_script2,harmonic_oscillator_sympnet}.jl — four scratch files committed by accident. ed30aee8 is titled "Bring the scripts to the new AdamOptimizerWithDecay" and does that, but it also added four new files, byte-identical to files that were untracked in the working tree, mentioned in neither the commit message nor the PR body nor the changelog. They do not look intended: enzyme.jl needs Enzyme, absent from scripts/Project.toml, and sae_script2.jl reads ../docs/src/tutorials/*.jld2 by relative path and writes a thousand PDFs. Removed from the repository; your local copies are untouched.
4. src/optimizers/optimizer.jl — the two step_size entry points disagreed. linesearch = Static(α) was accepted as a fixed learning rate, but step_size = Static(α) — the same request through the other keyword — fell off the end of _optimizer_step_size:
MethodError: no method matching _optimizer_step_size(::SimpleSolvers.Static{Float64})
which leaks an internal helper's name, and means the ArgumentError you wrote explaining that a training loop has no objective for a line search to search along was only reachable from one of the two paths. Anything that is not a plain number now goes through that single funnel.
Also folded in: the stale comment in test/…/multi_head_attention_stiefel_optim_cache.jl still claiming a blanket using would be ambiguous "since GeometricMachineLearning re-exports its own versions of them" — the very reason this branch removes; the test/exports.jl pointer in src/GeometricMachineLearning.jl, which is GeometricOptimizers' file, not one that exists here; six dead entries in docs/utils/adjust_image_size.jl for figures from the deleted pages (silent no-ops, since adjust_image_size matches nothing and moves on); and the GPU snippet in docs/src/tutorials/symplectic_autoencoder.md:137, which paired a Float32 network with AdamOptimizerWithDecay(integrator_train_epochs) — Float64 now that the type is positional rather than taken from η₁, a pairing OptimizerCache rejects, so the snippet could not have run as printed.
Pkg.test() (exit 0, 0 failures) and check_references.jl (0 unresolved) both still pass after all of it.
Not fixed — two things for you to decide
DecayingStatic's horizon is n_epochs, but opt.iterations counts batches. optimization_step! is called once per batch from the loop in src/data_loader/optimize.jl:69, so AdamOptimizerWithDecay(n_epochs) reaches η₂ after n_epochs batches, not epochs. In this repo's own decay test — n_epochs = 2048, 13 batches/epoch:
total optimization_step! calls |
26624 |
α reaches η₂ at |
epoch 158 of 2048 |
α at the final step |
1.0e-54 |
So about 92% of that run trains below the nominal floor, ending 48 orders of magnitude under it. This is pre-existing — main computes η₁·γ^t off the same per-batch counter, and so did the pre-0.5 code — so it is not a regression here, and I have not touched it. But this PR is what makes the (algorithm, linesearch) pairing the documented API, and n_epochs naming a batch count is the kind of thing this branch is otherwise very good about being straight on. Worth its own issue.
test/performance_tests/optimizer_update_gpu_test.jl:9 and test/performance_tests/optimizer_gpu_tests/adam_update_gpu_test.jl:10 call bare AdamCache, which the cache de-export makes an UndefVarError. I deliberately did not add the qualifying import the three reachable transformer_related/* files got, because it would be cosmetic: both files also call GeometricMachineLearning.convert_to_dev, which is itself one of the ten undefined exports; AdamCache(B₁, B₂) does not match upstream's (x::OptimizerSolution, g) arity; update!(o, cache, B) predates the current signature; and optimizer_update_gpu_test.jl uses T without defining it. Neither file is reachable from runtests.jl. They are C11 material, not a fixable regression — an import would just make dead code look maintained.
One note on the description
The PR body's file list and the "twenty-three test files" accounting are right, but note that GitHub's API truncates the changed-file list at 100 and this PR touches 110 — test/runtests.jl is among the ones that fall off the end. It is updated correctly (every deleted include is removed; I checked all 23 against it), but anyone reviewing through the API's file list rather than the diff will not see that and may reasonably think the suite is left pointing at deleted files.
|
Pushed the four fixes from my review as three commits on this branch:
Verified after the fixes: Two things I deliberately left alone, both explained in the review: the The title-page image in |
#240 moved the compat bound to `"0.3"` on main while this branch moves it to `"0.4"`, which is the one conflict. `"0.4"` wins: the interface this branch imports -- `metric`, `check`, `Ω`, `global_section`, `update_section!`, the retraction types, `AdamOptimizerWithDecay` -- only became public API in 0.4.0, and GML does not load against 0.3. Without this the two branches could not be merged, so GitHub could not compute a merge commit for the pull request and skipped every `pull_request` workflow: the branch had zero CI runs, not failing ones. #240's changelog entry announced the bump as a move to 0.3. Since 0.5.0 is the first release either bump appears in, that intermediate step is not something a caller ever sees, so the entry now describes the move as `"0.2.1"` → `"0.4"` and keeps the 0.3 analysis as the reason the step through it was safe. Its closing paragraph had also gone stale in the merge -- it argued that GO's `BFGS`/`DFP` exports could not collide because a blanket `using` would clash with the ~20 types GML defined itself, and this branch removes both the named `using` list and those types. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
One correction to my review, and it cuts against me. I wrote that the two LaTeX-pipeline breakages "would have been caught by CI, but only after this landed." That is wrong: the So the finding stands and matters more than I said, not less — the fix just has no CI to confirm it. I verified it directly instead: the recipe now exits 0 with Worth deciding separately whether Related, now that |
CI now runs, and it is red on both supported Julia versionsMerging The pattern is not flakiness. Across two full runs, on all three operating systems:
Two distinct tests fail, split cleanly by version:
For reference, Reproduced locally, and narrowedI reproduced it off CI: same machine, same Julia 1.10, same test matrix
The branch's numbers match CI's to every digit, so this is the same failure and not a local artifact. What I was able to rule out:
So the difference lives somewhere else in the migrated cayley path — Note also that geodesic + gradient differs at the last three digits ( What I did not doI deliberately did not touch the tolerances. Relaxing Everything else I reviewed stands: the four fixes I pushed are unrelated to this, Reproduction, if useful: check out this branch and |
Final CI verdict on
|
`svd_optim.jl` failed on Julia 1.10 and `sae_error_lower_than_psd_error.jl` on 1.12. Neither was a numerical regression: given the same starting point the new optimizer stack agrees with the old to 13 significant digits. Each file seeded once at the top and then called its helper twice, so the second call started from whatever RNG state the first happened to leave behind. GeometricOptimizers 0.4 builds one more `GlobalSection` per manifold parameter than 0.2 did -- GML's `StiefelManifold` *is* its type now, so its generic manifold machinery engages where `go_bridges.jl` used to -- and `global_section` calls `randn!`, so every draw after the first `Optimizer` construction shifted. Both tests were passing on a thin margin: the `svd_optim.jl` gradient run went from 2% above the optimum to 21%, against a 10% tolerance. Seeding each invocation makes the starting point independent of what ran before it. The two assertions now clear by 23x and by 2.6-4.5%, stable to 13 digits across 1.10, 1.12 and 1.13 -- 1.13 had been clearing the autoencoder comparison by 0.7%, i.e. by luck. `psd_optim.jl` and `adam_with_learning_rate_decay.jl` have the same shape and get the same treatment. The latter's manifold run also goes from 32 to 128 epochs: `AdamOptimizerWithDecay(n_epochs)` fixes γ = exp(log(η₂/η₁)/n_epochs), so a 32-epoch budget collapses the learning rate to η₂ before the run has trained, and from a seeded start the loss fell by under 2% on 1.13 and *rose* on 1.12. Full `Pkg.test()` is green on 1.10, 1.12 and 1.13 (56 testsets each). This had not been checked before: CI aborts at the first failing `@safetestset`, so most of the suite had never run on 1.10 or 1.12. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI on 1.10 and 1.12 — diagnosed and fixed in d5a2417Six of the nine red jobs on 0de38ba were not test failures:
Both reproduce bit-for-bit locally against the registered 0.4.0 (aarch64 macOS reproduces the Not a numerical regressionGiven the same starting point the new optimizer stack agrees with the old to 13 significant What changed is that # GeometricOptimizers/src/optimizers/named_tuple_wrapper.jl:126
_similar(a::Manifold{T}) where {T} = rand(manifold_constructor(a){T}, size(a)...)because upstream deliberately refuses the alternative: # GeometricOptimizers/src/manifolds/abstract_manifold.jl:177
Base.similar(::Manifold) = error("The function `similar` does not make sense in this context. Consider using rand.")
The Why that broke exactly these two testsBoth seeded once at the top of the file and then called their helper twice, so the second
1.13 was passing by luck, not by correctness: it cleared the autoencoder comparison by 0.7% The fixSeed at the start of each helper invocation, so the starting point is independent of what ran before
Also gone: the unused
VerificationFull The six infrastructure-failed jobs need a re-run regardless of this change. Corrected after posting. This section originally said the optimizer state builds "two |
The previous commit blamed the shifted random stream on the optimizer state
building "one more `GlobalSection` per manifold parameter". It does not: the
`global_section` calls are identical on both sides, four per `Optimizer` over
two Stiefel weights.
The extra randomness is `_similar`. `GeometricOptimizers._similar(a::Manifold)`
is `rand(manifold_constructor(a){T}, size(a)...)` -- a fresh random point on
the manifold -- because upstream makes `Base.similar(::Manifold)` an error on
the grounds that uninitialised storage is not a manifold point.
`GradientState` allocates its `x̄` slot with it. GML's `StiefelManifold` is
GeometricOptimizers' type now, so that method applies; on `main` the call fell
through to `similar(a)` and GML's own `Base.similar(::StiefelManifold)`, which
allocated uninitialised storage and drew nothing. Six batches of normals per
`Optimizer` where there were four.
The wrong claim came from a bad measurement: counting draws by locating the
next scalar `randn()` in a reference list, which assumes an array fill
consumes the stream the way scalar draws do. It does not, so those numbers
were meaningless. Counting `randn!`/`randn` calls and reading the Xoshiro
state directly is what settled it.
Nothing about the fix changes -- the tests are unchanged by this commit, and
the reason they needed seeding stands.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
- Coverage 66.50% 65.62% -0.88%
==========================================
Files 111 100 -11
Lines 3860 2985 -875
==========================================
- Hits 2567 1959 -608
+ Misses 1293 1026 -267 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The manifold optimizers were split out into GeometricOptimizers in #230, but the split stopped halfway. GML kept near-verbatim copies of eleven types defined upstream —
Manifold,StiefelManifold,GrassmannManifold,SkewSymMatrix,SymmetricMatrix,AbstractTriangular,LowerTriangular,UpperTriangular,AbstractLieAlgHorMatrix,StiefelLieAlgHorMatrix,GrassmannLieAlgHorMatrix,StiefelProjection— plus thirteen documentation pages and twelve test files covering them.This finishes it. Requires GeometricOptimizers 0.4.0, which is registered.
Why the copies were not harmless
Julia saw them as distinct types, so none of upstream's generic machinery dispatched on GML's.
geodesic(::Manifold, ::AbstractMatrix),cayley,global_rep,apply_section!andupdate_section!were each written again per manifold, andsrc/optimizers/go_bridges.jlheld about thirty more methods whose only job was reconnecting the two hierarchies. 2595 deletions against 202 insertions insrc/.Three bugs were live in GML's copies and already fixed upstream, so importing fixes them. Each was reproduced before and after:
rand(3,3) + SkewSymMatrix(...)was aStackOverflowError—Base.:+(B::AbstractMatrix, A::SkewSymMatrix) = B + Acalled itself.parent(::StiefelLieAlgHorMatrix)returned an unboundBwhere it meant(A.A, A.B).optimization_step!read it before incrementingopt.iterations, so a run's first step wasα(0) = η₁where the pre-0.5 code tookα(1) = γη₁. Every step of a run sat one place early in the schedule.importrather thanconst X = GeometricOptimizers.X: GML adds constructor methods to several of these types, and extending a type reached throughusingwarns on every such method since Julia 1.12.The export collisions
Twelve exported names resolved to different objects in the two packages, so
using GeometricMachineLearning, GeometricOptimizerswas anUndefVarErroron any of them — not just onAdamOptimizerWithDecay, which is all issue B1 recorded. One is left:Optimizer, which stays until the parameter-tree traversal moves upstream (see C1).Two of the twelve were not in the issue list and are worth calling out, since both change the exported surface:
update!had zero methods here. The export only shadowedGeometricOptimizers.update!— a different generic function, and the one that actually has methods for the optimizer caches.solve!was a second generic function of the same name.AdamOptimizerWithDecaywas the same algorithm in both, differing only in packaging. GML's struct is deleted. A call has to change: it is now an(algorithm, linesearch)pairing,Tis positional and defaults toFloat64rather than being taken fromη₁, andρ₁/ρ₂are the keywordsβ₁/β₂.Documentation
Thirteen pages move upstream — the whole
Manifoldschapter, the twoSpecial Arrays and ADpages whose types are upstream's, and the wholeOptimizerpart. They documented types that are no longer even defined here. Three pages split rather than moved:optimizer_framework.mdleaves behinddocs/src/optimizers/optimizer.mdfor GML's ownOptimizerand training loop, the Parallel Computation section folds intoarrays/tensors.md, andmanifolds.md's backend reference becomes KernelAbstractions'.Thirty-six references across the boundary stay references:
DocumenterInterLinksjoins the docs environment, which also closes C3.The inventory is read from a committed file, not a URL. A docs build that cannot run until an unrelated deploy has happened is a build that will break again for the same reason later. It is regenerated from the deployed 0.4.0 docs; the command is in a comment above
links = InterLinks(...).The book loses a part and a chapter, so
_latex_pages,docstring_index.mdand the prose inabstract.md/introduction.md/outlook.mdthat promised those chapters are all updated.Tests
Twenty-three files go: twelve duplicated upstream's suite, eight were unreachable from
runtests.jland could not have run (twoincludepaths deleted years ago, threeusing Lux), three tested behaviour that is now upstream's. What GML covered and upstream did not was ported there first — which turned up four defects in upstream's own suite, including astiefel_global_sectionthat built aGrassmannManifold, so the Stiefel global section had no test at all.Kept deliberately: everything that drives
Chain/NeuralNetwork/optimization_step!. Those look like upstream'ssvd_optim.jland are not — that one drivessolve!against an objective, these train a network with gradients from Zygote.A test that trains now passes
show_progress = false. The default is unchanged, so interactive use is unaffected; the 2048-epoch run alone was emitting a few hundred progress lines for a failure to hide in.Verification
All three gates run against the registered 0.4.0, not a path checkout:
Pkg.test()— passes, 0 failures, 0 progress lines.docs/check_references.jl— 0 unresolved.julia --project=docs docs/make.jl— exit 0, zero errors (down from 60 when the move first landed).using GeometricMachineLearning, GeometricOptimizersresolves every shared name exceptOptimizer, andGeometricMachineLearning.SkewSymMatrix === GeometricOptimizers.SkewSymMatrixholds for all eleven types.Also here
README.md; One plotting library, and the bugs that finding it uncovered #238 superseded its plotting hunk.[sources]entries removed so both environments resolve from the registry. Worth recording:Pkg.freefails with "could not find source path", andPkg.resolvewrites the entry back — the route that works is remove by hand, delete the gitignored manifests,instantiate.[sources] GeometricMachineLearning = {path = ".."}in the docs and scripts environments), replacing comments whose reasoning predates the[sources]feature.runtests.jl. Both measured rather than estimated; neither is closed here.Closes #234.
🤖 Generated with Claude Code