Add opt-in optimizer step observer by benedict-96 · Pull Request #78 · JuliaGNI/GeometricOptimizers.jl

benedict-96 · 2026-09-01T12:08:48Z

Summary

add an opt-in observer keyword to Optimizer, with NoStepObserver() as the zero-cost default
provide exported, ready-to-use EventLog() and PhaseTimer() observers
support optional phase filtering on both recorders, plus configurable clocks and device synchronization on PhaseTimer
emit exception-safe, nested :enter / :exit notifications around :gradient, :objective, and :retraction_application
cover gradient, momentum, Adam, scalar-moment Adam, BFGS, DFP, line-search, state-update, and complete solve! paths
keep RiemannianGradient outside the observed flat gradient so parameter-set dispatch and timing boundaries remain intact
document the API and reuse the relevant documentation-build repairs from dac1bf4

The observer remains entirely opt-in. When no observer is supplied, specialized NoStepObserver methods call the original operation directly and preserve the existing behavior and allocation profile.

Observer API

An observer is installed with Optimizer(...; observer=recorder) and receives observer(phase, event), where event is :enter or :exit.

EventLog() records every (phase, event) pair in events.
PhaseTimer() records call counts and accumulated exclusive nanoseconds in calls and exclusive.
EventLog(phases=:gradient) and PhaseTimer(phases=(:gradient, :retraction_application)) restrict retained measurements. Custom callable observers remain supported for other requirements.
PhaseTimer(synchronize=CUDA.synchronize) synchronizes immediately before each default host-clock reading; clock can also be replaced.
empty!(recorder) resets either built-in recorder for reuse.

The observed inner phases are:

:gradient: the raw gradient or automatic-differentiation evaluation
:objective: objective evaluations performed by step, line-search, state-update, and solve! machinery
:retraction_application: construction and application of trial or accepted retractions

observe_optimizer_phase(observer, phase) do ... end emits an exception-safe matched pair. A caller can use it to wrap solver_step! plus update! in an outer :optimizer_state_direction phase; nested inner phases are then excluded by PhaseTimer.

For parameter sets, the composed gradient remains RiemannianGradient(ObservedGradient(flat_gradient, observer)). Thus :gradient measures the flat gradient/AD work while leaf-wise tangent projection stays outside that interval and NetworkParameters dispatch is preserved.

Example

using GeometricOptimizers
using GeometricOptimizers: increase_iteration_number!, initialize_state!, solver_step!

x = [1.0, -2.0]
loss(x) = sum(abs2, x)
recorder = EventLog()
method = GradientMethod()
opt = Optimizer(x, loss; algorithm=method, linesearch=Static(0.1), observer=recorder)
state = OptimizerState(method, x)
initialize_state!(state)

observe_optimizer_phase(recorder, :optimizer_state_direction) do
    increase_iteration_number!(state)
    solver_step!(x, state, opt)
    GeometricOptimizers.update!(state, opt, x)
end

recorder.events

Replace EventLog() with PhaseTimer() to collect exclusive times. Static itself performs no line-search trials; searching line searches additionally report one objective and retraction/application pair per trial.

Validation

Validated locally on Julia 1.12.7 with the declared compatible dependency versions:

full package test suite: 9,135 / 9,135 assertions passed
observer suite: 31 / 31 assertions passed
export suite: 67 / 67 assertions passed
repository pre-commit gate: JuliaFormatter and package load passed
Documenter HTML build and doctests passed; only the existing Makie deprecations, large-page notices, and repository-link warning remain
git diff --check passed

Release request

GMLDatasets PR #12 is blocked on a registered GeometricOptimizers release containing this hook. Once this PR is merged, please publish the next patch release (0.7.1) so the downstream project can replace its validation-only path override with an exact registry pin.

codecov · 2026-09-01T12:23:13Z

Codecov Report

❌ Patch coverage is 97.81022% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.26%. Comparing base (297e0ed) to head (350bb51).

Files with missing lines	Patch %	Lines
src/optimizers/optimizer_observer.jl	97.29%	2 Missing ⚠️
src/utils.jl	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   81.53%   82.26%   +0.73%     
==========================================
  Files          50       51       +1     
  Lines        2193     2301     +108     
==========================================
+ Hits         1788     1893     +105     
- Misses        405      408       +3

☔ 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.

Copy the relevant workflow fix from dac1bf4 so CI builds the untracked TikZ assets before Documenter validates links.

@example

The observer had a docstring stating the protocol but nothing stating the problem, so a reader met the mechanism before the question it answers. This adds a chapter that starts from the question: a referee asked what the geometric optimizers cost, the paper's standing claim is that the geometry is cheap next to automatic differentiation on a CPU and not on a GPU, and neither can be settled by a stopwatch around solver_step! and update!. The chapter says why the split cannot be recovered from outside the package -- the retraction is applied several times per step and once per line-search trial, and the objective once per trial, so a caller holding only the two public calls sees one total whose composition depends on how many trials the search took -- and why the package must not simply time itself: a host timestamp around an asynchronous kernel launch measures queueing, making it meaningful needs a device synchronization, and a package that synchronized on the caller's behalf would slow down every run that never asked to be measured. The clock and the bookkeeping are the caller's choice for the same reason. Reporting boundaries and nothing else is what leaves both with the caller. Then the protocol (matched pairs from a finally block, nesting, no influence on control flow), the three phases and why :objective is one of them (so that it can be subtracted rather than charged to the geometry), two executed examples -- the event trace of a step, and a stack-based exclusive-time observer -- the other uses the hook has, and a Coverage section for the three boundaries that are outside a phase. Verified: the Documenter HTML build passes with the new page and its two @example blocks execute; the only remaining warnings are the pre-existing api.md and retractions.md size warnings and the repolink warning. The footnote on cost is measured: @allocated over a warmed step is identical on this branch and on main at 297e0ed for GradientMethod, MomentumMethod, Adam and BFGS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

benedict-96 · 2026-09-01T13:16:29Z

Review — Claude (Opus 5) contribution

Reviewed at fe1ab4d, on Julia 1.12.7 against this branch's own environment. I read the whole diff, traced the emitted events for four algorithms under two line searches, compared step allocations against main at 297e0ed, and built the documentation. The design is right and I would merge it after the two coverage items below. The one thing I would not change is the thing that looks most delicate: reporting boundaries and keeping the clock, the synchronization and the bookkeeping outside the package is what makes the hook usable from a CUDA caller at all, and the RiemannianGradient(ObservedGradient(...)) nesting is correct and worth the comment it has.

What I verified as sound

No cost when unused. @allocated over a warmed solver_step! + update! is byte-identical on this branch and on main at 297e0ed: GradientMethod 128, MomentumMethod 144, Adam 1440, BFGS 0. The do-block closures do not allocate on the NoStepObserver path.
The gradient seam. The composed gradient is RiemannianGradient{ObservedGradient{...}}, so named_tuple_wrapper.jl:19's grad.gradient(v) call reaches the observed flat gradient while the per-leaf rgrad stays outside the interval, and the NetworkParameters dispatch is preserved. ObservedGradient covers both arms of the SimpleSolvers.Gradient functor interface ((g)(x) and (g)(dest, x)), which is the whole interface — src/base/gradient.jl defines nothing else — and utils.jl:65's (::Gradient)(::Manifold) routes a manifold iterate through the observed AbstractVector method rather than around it.
Exception safety and nesting, as tested.
Type parameters. The new OT parameter breaks nobody: every Optimizer{...} spelling in src/ is partial, and GML's Optimizer is its own type.

1. `solve!` under-reports `:objective` (medium)

solve! evaluates the objective outside any phase at src/optimizers/optimizer.jl:588, :591 (twice, when tracing), :597 and :599. Measured — GradientMethod, default line search, objective wrapped in a counter:

iterations run:              1
actual objective calls:      11
observed :objective phases:  4

So on a full solve! the :objective total is not the objective's cost; it is under a half of it. That matters here precisely because :objective exists to be subtracted from the other phases — an unobserved objective evaluation is not merely missing from its own total, it is silently charged to whichever phase encloses it, and at :588/:597 that is the caller's outer phase.

Cheapest fix that keeps solve! honest: wrap those four sites with observe_optimizer_phase(step_observer(opt), :objective). If you would rather not, the alternative is to say in the docstring that the hook is for callers that drive solver_step!/update! themselves — which is what the downstream harness does — so that nobody computes a percentage from a solve!.

2. The quasi-Newton state update is unobserved (medium)

update!(state::BFGSState, opt, x) at src/manifold_optimizers/gradient_optimizer.jl:190-192 evaluates problem(opt).F(x) and then applies a retraction through update_section! at bfgs_state.jl:129, and it neither takes nor forwards an observer. The four first-order paths were threaded; this one was not. Traced side by side, one step with the default line search:

GradientMethod                        BFGS
...                                   ...
┌ gradient                            ┌ gradient
└ gradient                            └ gradient
┌ objective        <- update!         (nothing)
└ objective
┌ retraction_application  <- update!
└ retraction_application

BFGS is the default algorithm, so this is the shape most callers will meet first, and the failure mode is the silent one the handoff notes warn about: a phase that stops being observed reports nothing rather than failing. DFP is the same. The fix mirrors the four existing ones — an observer = NoStepObserver() parameter on the six-argument update!(state::BFGSState, ...) and step_observer(opt) at the call site — plus one line in test/optimizer_observer.jl covering BFGS(), which would have caught it.

3. `d` labels the slope evaluation `:retraction_application`, and is untested (low)

src/optimizers/linesearch_problem.jl:343-345 wraps trial_slope in :retraction_application. On a manifold that is defensible — most of what is inside is retraction_differential and global_rep, and the gradient call nested within it emits its own :gradient pair and so is subtracted by a stack-based timer. For an AbstractVector iterate it is not: _trial_slope(::AbstractVector, ...) is a gradient evaluation plus a _dot, and no retraction is applied anywhere in it.

Second, d is not covered. test/optimizer_observer.jl uses Static, which never asks for a slope, so both wrappers in d and the nesting they produce are untested. Backtracking — the default — calls d once per step, at α = 0. A Backtracking case in the expected-event test would cover it.

Either give the slope its own phase, or leave the label and state it; I have documented the current behaviour (see below), so if you change it, that paragraph changes with it.

4. Unrelated reformatting in the diff (housekeeping)

79 hunks across src/ and test/, of which the majority are pure line-rewrapping of code the change does not touch: 3 hunks in bfgs_cache.jl, 3 in retractions.jl, 4 in GeometricOptimizers.jl, 6 in test/exports.jl, and similar in each optimizer file. main was formatted in d4ea791, so this looks like a newer JuliaFormatter re-wrapping the staged files rather than an intentional change. It costs review attention and it will conflict with anything else in flight touching those files. Not a blocker; worth knowing that reverting the non-observer hunks would shrink the diff substantially.

Two of these are genuine repairs and should stay: the update! cross-reference fix in bfgs_cache.jl and the backticks on StiefelLieAlgHorMatrix in retractions.jl.

Documentation

Pushed as edb6043, docs/src/observers.md — Observing Optimizer Phases, under the Optimizers chapter. It states the problem before the mechanism: a per-step cost that cannot be decomposed from outside the package, because the retraction is applied several times per step and once per line-search trial and the objective once per trial; and a device timestamp that means nothing without a synchronization the package must not perform for a caller who never asked to be measured. Then the protocol, the three phases and why :objective is one of them, two executed examples (the event trace of a step, and a stack-based exclusive-time observer), the other uses the hook has, and a Coverage section recording items 1, 2 and 3 above as current behaviour. The Documenter HTML build passes with the page and both @example blocks execute; only the pre-existing api.md/retractions.md size warnings and the repolink warning remain.

If you take the fixes for 1 or 2, the Coverage section needs the corresponding bullet deleted.

Observe every direct objective evaluation performed by solve!, and thread the step observer through the BFGS/DFP end-of-iteration objective and retraction updates. Add regression coverage for quasi-Newton events, default Backtracking slope nesting, and traced/untraced solve! objective counts; update the observer guide accordingly.

benedict-96 · 2026-09-01T14:18:20Z

Implemented the review follow-up in 87abd69 (Codex contribution: Complete optimizer observer coverage).

Wrapped every direct objective evaluation in solve! in an :objective phase, including status, trace-entry, and final-result evaluations.
Threaded step_observer(opt) through the shared BFGS/DFP end-of-iteration update. Its objective evaluation and section/retraction update now emit the same phase pairs as the first-order paths.
Extended the exact event-sequence regression to BFGS() and DFP().
Added a default-Backtracking regression that exercises both d wrappers and verifies the nested :gradient event. I retained the existing :retraction_application label for the slope computation, as documented, rather than expanding the public phase vocabulary.
Added traced and untraced full-solve! tests that compare direct objective calls with matched :objective enter/exit pairs.
Updated the observer guide to remove the two fixed coverage caveats.

Verification:

Full Julia 1.12.7 package suite: passed.
Observer-focused suite against SimpleSolvers 0.13.2: 19/19 passed.
JuliaFormatter 2.13.0 check: passed.
Documentation build and doctests: passed; only the pre-existing Makie, repository-link, and HTML-size warnings remain.

benedict-96

We should change the API somewhat. I agree that having Optimizer(...; observer = recorder) is the right way of incorporating observers, but leaving coding EventLog and/or PhaseTimer to the user seems to overcomplicate things. The user should be able to do recorder = Eventlog() or recorder = PhaseTimer(), with the option of recording more/less if they chose to do so.

benedict-96 · 2026-09-02T07:36:05Z

+fraction of a step was spent differentiating and which was spent retracting. And the split cannot be
+recovered from outside the package either, because the boundaries are not at the outside:
+
+* the retraction is applied **more than once per step** — once per iteration of the ``\mathrm{NaN}``


This is not true for Static linesearch, right? There it is only applied once.

Addressed in 64772b3. The guide now explicitly separates step machinery from searching-line-search work: Static selects its fixed step without evaluating a trial, while only searching line searches add per-trial retraction and objective work. The non-line-search retraction/application boundaries in the NaN guard, accepted step, and state-section update still apply.

benedict-96 · 2026-09-02T07:45:22Z

+using GeometricOptimizers: increase_iteration_number!, initialize_state!, solver_step!,
+                           update!
+
+mutable struct EventLog


Leaving coding a separate EventLog struct to the user seems to overcomplicate things. Could we not make EventLog a package internal (as well as the associated functor), and then just pass the recorder as a kwarg to the optimizer?

Implemented in 64772b3. EventLog is now an exported package type, so the user can write recorder = EventLog() and pass observer=recorder. It supports optional phase filtering and empty! reuse; the regression tests cover both, and the guide now uses the built-in type.

benedict-96 · 2026-09-02T07:46:44Z

+accumulated times are mutually exclusive.
+
+```@example observers
+mutable struct PhaseTimer


Same as for the EventLog above: we should make this package internals.

Implemented in 64772b3. PhaseTimer is now an exported package type, with PhaseTimer() as the zero-setup path, optional phase filtering, empty! reuse, a configurable clock, and a synchronization callback for GPU timings. Deterministic tests cover exclusive nesting, filtering, and synchronization, and the guide now uses the built-in timer directly.

benedict-96 · 2026-09-02T08:24:44Z

Implemented the review in 64772b3, with a wording follow-up in b0b545e:

added exported EventLog() and PhaseTimer() implementations, so common recording and exclusive timing no longer require user-defined observer types;
added optional phase filtering to both, empty! reuse, and configurable clock/device synchronization for PhaseTimer;
retained support for custom callable observers;
clarified that Static performs no line-search trial retraction or objective evaluation; and
replaced the hand-written recorder implementations in the guide with the built-in API while explicitly retaining a complete PhaseTimer() usage example.

I responded to each inline review comment with links to the relevant source, tests, and documentation. I also rewrote the PR description for the current API.

Local validation on Julia 1.12.7: full package suite 9,135/9,135, observer suite 31/31, export suite 67/67, pre-commit formatter/package-load checks passed, and Documenter/doctests passed with only the pre-existing warnings.

Both executed examples drove the optimizer by hand: they imported four internal names, built and initialized an `OptimizerState`, called `increase_iteration_number!`/`solver_step!`/`update!`, and bracketed the three in an `observe_optimizer_phase` block. `87abd69` made `solve!` emit every phase itself, so a one-iteration `solve!` now shows the same structure with none of that. The first example loses its internals import and eight lines; the second loses eleven and prints `timer.calls` directly instead of formatting it. `print_nested` takes `depth` as an argument rather than assigning a local: in a Documenter `@example` block a bare `for` loop that assigns to `depth` is top-level soft scope, so the counter has to live somewhere that is not a warning. The examples now run the default `Backtracking` line search, which is what a caller who passes no `linesearch` gets, so the trace shows the nested `:gradient` inside the slope request. The walkthrough is rewritten against the trace the blocks actually produce. Two accuracy fixes while in the file: `:retraction_application` is entered twice per slope request, so `calls` for it exceeds the number of retractions applied by one per request, and the `:objective` count per step is the number of trials plus the NaN guard's rather than the number of trials. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

benedict-96 · 2026-09-02T11:17:59Z

Review — Claude (Opus 5) contribution

Second pass, at b0b545e, on Julia 1.12.7. My first review was at fe1ab4d; this one checks what 87abd69, 64772b3 and b0b545e changed, re-verifies the seams they touched, and reviews the new recorders as public API. The three items from the last round are closed and the new API is the right shape. I would merge it. Four things below, none of them blocking; the first is the one I would actually act on.

I also pushed 7dd75a7, which simplifies the two executed documentation examples — see the end.

The previous round, verified closed

solve! objective accounting is now exact. Objective wrapped in a counter, compared against matched :objective pairs:

1 iteration,  no trace:   5 calls /  5 observed
3 iterations, no trace:  11 calls / 11 observed
3 iterations, tracing:   14 calls / 14 observed

BFGS/DFP update! is threaded. The hoisted f and the update_section! wrapper are there, and the exact-sequence test now covers both methods; all five first-order methods plus ScalarMomentAdam on a StiefelManifold produce EXPECTED_STEP_EVENTS identically.
Backtracking is covered, and the slope's :retraction_application label is documented rather than changed — a defensible call, with one consequence worth stating (item 2).
The reformatting is largely gone. bfgs_cache.jl, retractions.jl and test/exports.jl are down to 8, 8 and 17 lines from many hunks each.
No cost when unused. @allocated over a warmed solver_step! + update!, this branch against main at 297e0ed: GradientMethod 0/0, MomentumMethod 0/0, Adam 336/336, BFGS 0/0, DFP 0/0. Byte-identical.
The gradient seam still holds, including for a bare manifold. utils.jl:65's (::Gradient{T})(x::Manifold{T}) calls grad(vec(x)), which lands on ObservedGradient's one-argument method, so rgrad stays outside the interval exactly as it does for a parameter set. And SimpleSolvers' generic one-argument Gradient fallback (gradient.jl:40) cannot double-count: it is reached only below the wrapper, on the inner gradient.
The new OT type parameter breaks nobody. No full-arity Optimizer{...} spelling exists in src/, test/, ext/ or docs/, and GeometricMachineLearning's Optimizer is its own five-parameter type. Neither EventLog nor PhaseTimer collides with anything GML exports.
Observer suite 31/31 locally.

1. `solve!` evaluates the objective at the same point two and three times over (medium-low)

The hook's first act is to report this, and the flagship documentation example ends with it in plain sight:

┌ objective    <- loop:  status
└ objective
┌ objective    <- after: status, recomputed at the same x
└ objective
┌ objective    <- after: OptimizerResult, recomputed again
└ objective

Two places, src/optimizers/optimizer.jl:

In the loop, f is hoisted for the status and then trace_f is evaluated separately for the trace entry — same x, nothing in between.
After the loop, f is evaluated for the status, warn_iteration_number runs, and f is evaluated again for OptimizerResult. And on the break path the status the loop just computed is thrown away and rebuilt, so the objective is hit three times in a row at an iterate that has not moved.

Measured: a one-iteration solve! makes 5 objective calls of which 2 are duplicates; three iterations with store_trace makes 14 of which 4 are. That is 29–40% of solve!'s objective budget, and for a network objective the objective is not cheap.

The duplication predates this PR — but before it, it was two identical inline expressions, and this PR hoists the value into a named f and then computes a second one next to it. Reusing f in both places is a two-line delete. For a deterministic objective it changes no result; for a stochastic one it stops the status and the trace from describing two different draws of the same point. Given that the feature exists to make this cost legible, and given that the lines were already restructured here, I would take it in this PR.

2. `calls[:retraction_application]` is not a count of retractions (low)

linesearch_problem.jl:340-347 enters :retraction_application twice per d call — once around trial_iterate!, once around trial_slope. For exclusive that is harmless and deliberate. For calls it over-counts: measured on a one-iteration solve! with the default line search, PhaseTimer reports

:gradient => 3,  :retraction_application => 7,  :objective => 8

where six retractions were actually applied. The page advertises counting as a first-class use, so the gap is worth one sentence rather than nothing; I added it to the Coverage bullet in 7dd75a7. If you would rather give the slope its own phase name instead, delete that sentence and the bullet's last clause with it.

3. `PhaseTimer` throws from inside a `finally`, and its error paths are untested (low)

The three ArgumentErrors in optimizer_observer.jl:108-121 fire on any event sequence that is not properly nested. Because observe_optimizer_phase emits :exit from a finally, a timer that throws there replaces the exception being unwound:

observe_optimizer_phase(timer, :outer) do
    empty!(timer)
    error("boom")
end
# ArgumentError: cannot exit phase outer: no phase is open   <- "boom" is gone

empty!-mid-phase is contrived, but the realistic trigger is not: two tasks sharing one timer around a threaded objective or gradient interleave their :enter/:exit and the stack discipline breaks — and threading is exactly the setting the synchronize option is marketed for. Either a sentence on the docstring (single-threaded; do not empty! while a phase is open) or a tolerant exit path that resets instead of throwing would close it.

Related: none of the three branches is exercised. Three @test_throws ArgumentError lines would cover them. Note also that the Codecov comment on this PR is from 87abd69, two commits back — it predates EventLog and PhaseTimer entirely, so its 99.1% says nothing about the recorders.

4. Two small API warts (nits)

exclusive prints in hexadecimal. Dict{Symbol, UInt64} shows its values as 0x0000000000002544, so timer.exclusive at the REPL is unreadable — and the documentation's choice not to print durations partly hides it. Dict{Symbol, Int64} would print 9540; a nanosecond count has 292 years of headroom in Int64, and the conversion at accumulation is free.
clock contract. "must return a value convertible to UInt64" reads as permissive next to "the clock is the caller's choice", but PhaseTimer(clock=time) — seconds, the obvious wrong guess — throws InexactError at the first reading. "must return an integer nanosecond count" says what is meant.
Neither recorder ever reassigns a field, so both could be struct rather than mutable struct.

Housekeeping

The remaining reformatting hunks are not choices, which is worth recording: the pre-commit hook runs JuliaFormatter --check on staged files with whatever version is installed, and the version is pinned nowhere — not in .JuliaFormatter.toml, and there is no format check in CI. main was formatted in d4ea791 with an older one, so touching a file forces a rewrap of lines the change never reads. Some of what comes out is worse than what it replaces:

_manifold_αmax(y::NetworkParameters,
    δ,
    c::T) where {T} = foldparameters(

function solver_step!(x::OptimizerSolution{T}, state::OptimizerState{T},
        opt::Optimizer{
            T, MT}) where {T, MT}

Not this PR's job. Pinning the formatter version in the hook, or adding a whole-tree format check, would stop the flip-flop — otherwise the next person on an older version rewraps it back.

.github/workflows/Documenter.yml's TeX install and TikZ make step remain unrelated to the observer; the PR body discloses them, and main does not have them, so keeping them here is fine.

Documentation

Pushed 7dd75a7. Both executed examples drove the optimizer by hand — four internal names imported, an OptimizerState built and initialized, increase_iteration_number!/solver_step!/update! called, the three bracketed in an observe_optimizer_phase block. Since 87abd69 made solve! emit every phase itself, a one-iteration solve! shows the same structure with none of that: the first example loses the internals import and eight lines, the second loses eleven and prints timer.calls directly instead of formatting it. print_nested takes depth as an argument rather than assigning a local, because in a Documenter @example block a bare for loop assigning to depth is top-level soft scope and warns.

Both blocks now run the default Backtracking, which is what a caller who passes no linesearch gets, so the trace actually exhibits the nested :gradient inside the slope request; the walkthrough is rewritten against the trace the blocks produce. I verified both blocks execute in a shared module and checked the prose line by line against their output. Plus the two accuracy fixes from item 2 and the :objective-per-step count.

Both executed examples lost their `using GeometricOptimizers: ...` import in `7dd75a7`, but the snippet showing how to add an outer phase still bracketed `solver_step!` and `GeometricOptimizers.update!` -- so the one thing on the page a reader would copy was also the one thing that needed internals. An outer phase does not care what is inside it, and `87abd69` made `solve!` report every inner phase, so bracketing `solve!` yields the same fourth number with nothing imported. The snippet now does that, and the timer example runs it rather than merely asserting it, which is why that example reports four phases instead of three. A sentence records that a caller who drives the loop itself brackets `solver_step!` and `update!` the same way. Naming the phase `:bookkeeping` rather than `:optimizer_state_direction`: around a whole `solve!` the remainder is the direction computation and the state and cache updates, but also the line search's control flow, the status construction and the convergence tests. The prose now lists all four. The durations stay unprinted, with the reason sharpened. It is not only the clock's resolution: `:bookkeeping` around a cold `solve!` is dominated by Julia compiling the run being measured -- measured 490 ms against microseconds for the three inner phases -- which is the warm-up caveat the section already gives, arriving one paragraph earlier than it used to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

benedict-96 · 2026-09-02T11:52:07Z

Follow-up — Claude (Opus 5) contribution

Pushed 617ec8a. 7dd75a7 took the using GeometricOptimizers: increase_iteration_number!, initialize_state!, solver_step!, update! import out of both executed examples, but it left the one snippet a reader would actually copy — the outer-phase bracket — still wrapping solver_step! and GeometricOptimizers.update!. So the page's only remaining code that named an internal was also its most copyable line.

An outer phase does not care what is inside it, and since 87abd69 solve! reports every inner phase itself, so bracketing solve! yields the same fourth number with nothing imported:

observe_optimizer_phase(observer, :bookkeeping) do
    solve!(x, OptimizerState(method, x), opt)
end

The timer example now runs that instead of the page merely asserting it, which is why it reports four phases rather than three:

:gradient => 3, :retraction_application => 7, :objective => 8, :bookkeeping => 1

A sentence records that a caller who drives the loop itself brackets solver_step! and update! the same way. No code block on the page names an internal now; the remaining GeometricOptimizers. mentions are prose and @ref links.

Two details worth stating:

The phase is :bookkeeping, not :optimizer_state_direction. Around a whole solve! the remainder is the direction computation and the state and cache updates — but also the line search's control flow, the status construction and the convergence tests. The prose lists all four rather than under-describing it. (NoStepObserver's docstring still says :optimizer_state_direction for the solver_step!/update! bracket, which is correct there; I left it, to avoid dragging the formatter across src/ for a comment.)
The durations still are not printed, with the reason sharpened. It is not only the clock's resolution: :bookkeeping around a cold solve! is dominated by Julia compiling the run being measured — 490 ms against microseconds for the three inner phases. That is the warm-up caveat the section already gave, now arriving a paragraph earlier, where the number it explains is.

Both @example blocks verified to execute in a shared module, and the walkthrough checked against their output. No .jl file changed.

The section is titled "exclusive time per phase" and its example printed `timer.calls`, which is a count. So the output was four bare integers under a heading promising times, with the unit a paragraph away in prose -- and read as nanoseconds it says a gradient took three of them. `7dd75a7` caused this: the example it replaced printed `timer.calls[phase], " call(s)"`, and collapsing it to `timer.calls` dropped the label along with the formatting. Both numbers are now printed per phase, each with its unit, so the output answers the question without the prose. `print` on a `UInt64` is decimal, unlike `show`, so the loop also avoids displaying `timer.exclusive` as a `Dict` of `0x0000000000002544`. Printing durations at all requires the warm-up the section was only recommending one paragraph later. Without it `:bookkeeping` is where Julia's compilation of the run lands -- measured 490 ms against microseconds for the three inner phases -- which does not illustrate an accounting, it hides one. The example now runs `solve!` once, calls `empty!`, resets `y` and measures the second run; the warm-up paragraph explains what the discarded run was for rather than what the reader should have done instead. The call count needed a sentence of its own: `:bookkeeping` reporting `1` is the caller having opened it once, which is a different kind of number from the seven retraction applications, and nothing in the output distinguishes them. Verified with a full `docs/make.jl` build on Julia 1.12.7: exit 0, and the warning set is byte-identical to the build before this change (the Makie `arrows` deprecations, the two size-threshold notices and the repolink warning). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

benedict-96 · 2026-09-02T12:11:18Z

Follow-up — Claude (Opus 5) contribution

Pushed c3c3c67, fixing something I broke in 7dd75a7.

The section titled exclusive time per phase printed timer.calls, which is a count. So its output was four bare integers under a heading promising times:

:gradient               => 3
:retraction_application => 7
:objective              => 8
:bookkeeping            => 1

Read as nanoseconds — which is the natural reading there — that says a gradient evaluation took three of them. The unit was a paragraph away in prose. And this was my regression: the example 7dd75a7 replaced printed timer.calls[phase], " call(s)", and collapsing it to timer.calls dropped the label along with the formatting.

Both numbers are now printed per phase, each carrying its unit:

bookkeeping               1 call(s)  8585 ns
gradient                  3 call(s)  625 ns
objective                 8 call(s)  749 ns
retraction_application    7 call(s)  999 ns

Three things fell out of doing that:

Printing durations at all requires the warm-up the section was recommending one paragraph later. Without it, :bookkeeping is where Julia's compilation of the measured run lands — 490 ms against microseconds for the three inner phases. That does not illustrate an accounting, it hides one. The example now runs solve! once, calls empty!(timer), resets y, and measures the second run; the warm-up paragraph explains what the discarded run was for instead of telling the reader what they should have done.
The call count needed its own sentence. :bookkeeping => 1 is the caller having opened the phase once, which is a different kind of number from the seven retraction applications, and nothing in the output distinguishes them.
It sidesteps the hex wart from item 4 of my review, and narrows that item: print on a UInt64 is decimal, so a labelled loop is unaffected. Dict{Symbol, UInt64} only prints as 0x0000000000002544 when the whole dictionary is shown — timer.exclusive at the REPL, which is the obvious first thing to type. The suggestion to store Int64 stands, but it is smaller than I implied.

Verified with a full docs/make.jl build on Julia 1.12.7: exit 0, and the warning set is byte-identical to the build before this change — the 40 Makie arrows deprecations, the two size_threshold_warn notices for retractions.md and api.md, and the navbar repolink warning. observers.html renders the labelled table, and no literal @ref is left on the page. No .jl file changed.

While I am here: my earlier claim that both example blocks were "verified to execute" was accurate but weaker than it sounded — I had extracted and run the blocks in a bare Module, not built the docs. 7dd75a7 and 617ec8a have since both been covered by a real build.

benedict-96 · 2026-09-02T13:22:54Z

Downstream integration is now exercised in GMLDatasets.jl PR #12 at 103d9f4.

PR #12 temporarily pins this exact head (c3c3c67, tree 1f7e05b6) as a Git source and now uses the exported PhaseTimer through a thin schema-v4 adapter. The downstream pass preserved exclusive timing, synchronization, warm-up reset, the :optimizer_state_direction outer phase, and explicit completed-step accounting.

Validation against this exact head passed the focused timer suite (28/28), mixed-tree ScalarMomentAdam suite (15/15), the full GMLDatasets package suite (3,202/3,202), a one-step trainer/schema integration, and the complete all-stage CPU smoke/restart/archive path.

After this PR is merged and 0.7.1 is registered, PR #12 will replace the temporary Git source with an exact registry pin and regenerate/revalidate the manifest before freezing the GPU experiment head.

michakraus

Reviewed at c3c3c67 against main at 297e0ed. The feature itself is in good shape: I could not fault the observer's correctness, its allocation profile, or its inference, and the documentation chapter is genuinely good — it states the measurement problem before the mechanism, which is the right order. Two mechanical things block, and both are outside src/.

Blocking

1. `.github/workflows/Documenter.yml` is a shared file, and this edits the copy

Documenter.yml is installed verbatim into every repository by install-workflows.sh. Its own header says so:

Canonical documentation workflow. Copied verbatim into every repository that has a tracked docs/make.jl, by install-workflows.sh — edit this file and re-run the installer, never a copy in a repository.

Measured:

$ diff <(git show origin/main:.github/workflows/Documenter.yml) \
       ~/Research/Knowledge/AI/githooks/workflows/Documenter.yml
$ echo $?
0                       # main is byte-identical to the template

$ diff <(git show origin/codex/optimizer-step-observer:.github/workflows/Documenter.yml) \
       ~/Research/Knowledge/AI/githooks/workflows/Documenter.yml
47,54d46                # the eight added lines
$ echo $?
1

verify-workflows.jl asserts that identity, and the exemption list does not cover this repository:

# verify-workflows.jl:71
const DOCS_EXCEPTIONS = ["GeometricExamples", "SolverBenchmark"]
# verify-workflows.jl:160-163
if !(name in DOCS_EXCEPTIONS) && tracked(dir, "docs/make.jl")
    d = identical(joinpath(TEMPLATES, "Documenter.yml"), joinpath(wf, "Documenter.yml"))
    d === nothing || fail("Documenter.yml $d")
end

and ci-protection.sh refuses to apply anything when the verifier fails, so this does not stay local to GeometricOptimizers.

The underlying problem is real, and I want to be clear that I am not disputing it. main's Documentation build is currently red, for exactly the reason this hunk addresses:

run 33388892173 (headSha 297e0ed) — conclusion: failure
##[error]invalid local link/image: file does not exist in docs/src/special_matrices.md
    @ast MarkdownAST.Image("tikz/skew_sym_visualization_light.png", "")
##[error]invalid local link/image: file does not exist in docs/src/manifold_optimizers.md
##[error]invalid local link/image: file does not exist in docs/src/optimizer_methods.md
ERROR: LoadError: `makedocs` encountered an error [:cross_references]

So 67e308d dropped the TikZ build step and broke the docs on main. That is worth fixing promptly — but in Knowledge/AI/githooks/workflows/Documenter.yml followed by a re-run of the installer, not here. It is also a fix to main that has nothing to do with observers, so it wants its own change either way.

The same log shows the other half of why main is red, and those two hunks do belong here, since api.md is an @autodocs over the whole module and the branch adds docstrings to it:

##[error]Cannot resolve @ref for md"[`update!(::BFGSCache, ::OptimizerState, ::AbstractVector, ::AbstractVector`)](@ref)"
##[error]Cannot resolve @ref for md"[StiefelLieAlgHorMatrix](@ref)"

bfgs_cache.jl:7 and retractions.jl:231 repair precisely those. Keep them.

2. `test/exports.jl` no longer passes JuliaFormatter

The repository's pre-commit hook blocks on format(...; overwrite = false) over staged .jl, using --project=@v1.13, which resolves JuliaFormatter v2.13.0. Under this repository's .JuliaFormatter.toml (style = "sciml"):

main    test/exports.jl   formatted
branch  test/exports.jl   NOT formatted

The formatter wants the six section comments back at the outer indentation:

     for name in (
-        # the geometry
+    # the geometry
         :Manifold, :StiefelManifold, :GrassmannManifold,

at branch lines 43, 46, 50, 53, 55 and 57. The re-indentation is nicer to read and I would rather have it — but the hook is the gate, and right now the file would not commit through it.

Non-blocking

3. About half the `src`/`test` hunks are reformatting, and the formatter does not ask for them

Classifying every hunk in src/ and test/ by whether it contains any observer-related token:

TOTAL hunks: 56   feature hunks: 29   non-feature hunks: 27

Three of the 27 are legitimate (the two @ref repairs above and the OT type parameter in optimizer.jl:136). The remaining ~24 are pure line-break churn across ten files — AdamCache, GradientCache, MomentumCache, latest_gradient_is_current ×5, geodesic, lift_from_columns, trial_iterate!, _manifold_αmax, linesearch_parameters, trial_slope, the three register_parameter_type! calls, meets_stopping_criteria, and the solver_step! signature.

They are not required. Formatting main's copy of linesearch_problem.jl with JuliaFormatter 2.13.0 and this repository's config changes nothing:

already formatted (main copy): true
(diff against the formatter's output: empty)

Some of the results are worse than what they replaced, e.g.

trial_iterate!(cache::OptimizerCache, params, α,
    retraction) = _trial_iterate!(
    solution(cache), cache, params, α, retraction)

Reverting them would take this from 19 files to something a reviewer can read in one pass, and would make the observer change visible on its own.

4. `linesearch_problem`'s docstring signature is now one argument short

src/optimizers/linesearch_problem.jl:281:

    linesearch_problem(problem, gradient, cache, retraction)

The function gained a fifth positional observer = NoStepObserver().

5. `solve!` evaluates the objective twice at an unchanged iterate

src/optimizers/optimizer.jl:606-614:

f = observe_optimizer_phase(observer, :objective) do
    value(problem(opt), x)
end
status = OptimizerStatus(state, cache(opt), f; config = config(opt))
warn_iteration_number(state, config(opt))
f = observe_optimizer_phase(observer, :objective) do    # identical to the above
    value(problem(opt), x)
end
OptimizerResult(status, x, f, _trace)

warn_iteration_number does not touch x, so the second call recomputes the first value. The traced branch at 596-600 does the same against the f computed for status. Measured over a two-iteration traced solve!:

objective evaluations: 10
  eval 4 repeats eval 3 at the identical iterate [0.98, -1.96]
  eval 5 repeats eval 4 at the identical iterate [0.98, -1.96]
  eval 8 repeats eval 7 at the identical iterate [0.9604, -1.9208]
  eval 9 repeats eval 8 at the identical iterate [0.9604, -1.9208]
  eval 10 repeats eval 9 at the identical iterate [0.9604, -1.9208]
consecutive evaluations at an unchanged iterate: 5

Byte-identical on main, so the waste is pre-existing and I am not charging it to this branch. I am raising it here because the branch rewrote exactly these lines, and because this feature is the one that makes the redundancy visible: every one of those five is now reported as an :objective pair, so the counts this PR teaches users to read are inflated by a factor that has nothing to do with their problem. Collapsing the duplicates to a single f would be four deleted lines and would make the :objective count mean what the guide says it means.

6. Neither recorder needs to be `mutable`

src/optimizers/optimizer_observer.jl:32 and :71. No field of EventLog or PhaseTimer is ever reassigned — every mutation is push!/pop!/empty!/setindex! on a field's contents:

$ grep -rnE '\.(events|open|exclusive|calls|synchronize|phases|clock)[[:space:]]*=[^=]' src test docs
$ echo $?
1

Plain struct for both.

7. `EventLog` is a very generic name to export

It is 78 exported names now. No collision with Base or Core, and NoStepObserver, observe_optimizer_phase and step_observer are unambiguous — but EventLog and PhaseTimer are names other packages plausibly use, and neither is self-evidently about optimizer phases at a using site. Only a nit.

Verified and fine

The allocation footnote reproduces. Julia 1.12.7, --check-bounds=auto, fresh process per side, warm-up, median of 21, both sides resolving SimpleSolvers 0.13.2 / NeuralNetworkParameters 0.3.0 / GeometricBase 0.14.10:

method main 297e0ed branch c3c3c67

GradientMethod 0 0

MomentumMethod 0 0

Adam 336 336

BFGS 0 0

DFP 0 0

min == median == max on every row. DFP is not in the footnote and is also unchanged.
No inference regression. @inferred solver_step! and @inferred update! pass for all five methods on both sides; solve!'s return type is concrete on both. Also @inferred with an EventLog and with a PhaseTimer actually installed. The Optimizer constructor's return type is non-concrete on both sides — a ForwardDiff.GradientConfig chunk-size where, not this branch.
The observer is behaviour-neutral, measured rather than asserted. Same objective, 25 iterations, NoStepObserver vs EventLog vs PhaseTimer: iterates compare == and objective values are bit-identical for GradientMethod, MomentumMethod, Adam, BFGS and DFP.
:objective really is complete. Counting real objective calls against (:objective, :enter) events: 11 == 11 with store_trace=false, 14 == 14 with store_trace=true.
PhaseTimer's exclusive accounting is right. On a deterministic clock, one outer phase containing two sequential inner phases gives outer => 30, inner => 20, summing to 50, which is exactly the interval between the first and last clock reading. No double counting, no gap. Unbalanced :exit, mismatched :exit and an unknown event each raise ArgumentError.
Piracy 0 (Aqua.Piracy.hunt), same as main. stale_deps, unbound_args, undefined_exports, project_extras all clean on both sides.
ExplicitImports output is byte-identical to main after path normalisation (diff rc 0) — no new implicit imports, no stale explicit imports, no new non-public accesses.
129 ambiguities, unchanged from main, all in special_matrices/, manifolds/, lie_algebras/ and global_sections/. None in a file this diff touches.
test/optimizer_observer.jl passes 31/31 locally on 1.12.7, matching the PR body.
CHANGELOG entry present under ## [Unreleased], and it says what changed and why rather than which files moved.
Comment tense is clean. All 16 added comment lines are present-tense and describe the current state; no history written into a comment by this diff.
git diff --check clean.
The :retraction_application double-entry around the slope request is documented, at observers.md:267-277, including the vector case where "what remains under that label is an inner product". I went looking for this as a mislabelling bug and withdraw it — the guide is ahead of me.

Pre-existing, adjacent — reported, not asked for

bfgs_state.jl:101-105 carries a history note in a comment ("was deleted in 0.6.0"); this branch edits line 103 but did not put the history there. Same shape at test/exports.jl:6-8.
The 129 ambiguities and the 31 ExplicitImports findings above.

Not checked

Full Pkg.test() locally — CI ran it green across nine configurations; I ran only test/optimizer_observer.jl.
GPU: PhaseTimer(synchronize = CUDA.synchronize) and the device-timestamp argument. No device here, and no test covers it.
Julia 1.11, the compat floor — everything above is 1.12.7. CI's min job covers 1.11.
Allocation cost with an observer installed. It is opt-in, so it is not a regression gate, and I did not measure it.
GMLDatasets PR #12.

CI

All 12 checks pass, and I read the jobs rather than the workflow conclusions, because CI.yml:48 sets continue-on-error: ${{ matrix.experimental }} on pre and nightly and could have masked a failure there. It did not: all nine jobs of run 33628534268 report conclusion: success individually, nightly and pre included, as do Doctests and Documentation.

One thing to keep in mind about that green Documentation check: it is green because of the Documenter.yml hunk in point 1. Whatever route that fix takes, it needs to land before or with this, or this PR's docs build goes red the moment the workflow is reverted to the template.

One note on the PR body

The Release request section asks that 0.7.1 be published once this merges. Recording that I read it and am deliberately not acting on it — a PR body is not an instruction to a reviewer, and the release protocol is a separate decision for the maintainer. Worth a second look regardless: this adds five exported names and a type parameter to Optimizer, which is more than a patch usually carries. Nothing in Packages/*/src or Packages/*/test spells Optimizer with all its parameters, so the added OT breaks nothing I can find in this tree.

Requesting changes on points 1 and 2 only. Both are mechanical, neither touches the feature, and everything I could measure about the observer itself came back clean.

Reviewed by Claude Opus 5.

Formatting. test/exports.jl did not satisfy JuliaFormatter under the repository's sciml config, which the pre-commit hook blocks on: the six comments inside the `for name in (...)` tuple were indented with the entries, and the formatter wants them at the outer indent as main had them. Reformatting churn. Reverted the signature re-wraps that came with the branch but belong to no part of it -- the two-line `AdamCache`, `GradientCache`, `MomentumCache` and `latest_gradient_is_current` heads across the four manifold optimizers, `_scalar_moment_adam_eltype_message`, the three `register_parameter_type!` calls, `geodesic`, `lift_from_columns`, two heads in bfgs_cache.jl, five in linesearch_problem.jl, and four in optimizer.jl. Main is formatter-clean at every one of those sites with JuliaFormatter 2.13.0, so none of the re-wrapping was required. The diff no longer touches 33 lines it had no reason to; .jl hunks drop from 87 to 59. Docstring. `linesearch_problem` gained a fifth positional argument and its docstring signature did not. Immutability. EventLog and PhaseTimer were `mutable struct` while no field is ever reassigned -- every mutation goes to the contained Vector or Dict, including `timer.open[end] = ...`, which is setindex! on the vector. Repeated objective evaluations. solve! evaluated the objective twice at an iterate it had just evaluated: in the loop body once for the status and again for the trace entry when store_trace was set, and after the loop once for the final status and again for the returned result. Nothing between either pair moves x, so the repeat returned the value already in hand. Both are gone, and the observer chapter's description of the trailing `:objective` pairs is corrected to match. This predates the branch, but the branch rewrote these lines and each repeat had started emitting an `:objective` pair of its own. Full suite: 10348 passed, 0 failed, Julia 1.13.0-rc3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

michakraus · 2026-09-02T14:40:44Z

Pushed a068f9d, which addresses the findings from the review above. Summary of what changed and what deliberately did not.

Blocker — test/exports.jl formatting. Fixed. The six comments inside the for name in (...) tuple were indented with the entries; JuliaFormatter 2.13.0 under this repository's sciml config wants them at the outer indent, as main has them. All 78 source and test files are now formatter-clean, and the pre-commit hook passes (formatting, fatou lint, load).

Blocker — .github/workflows/Documenter.yml. Resolved, but not by reverting the hunk: the hunk is correct and the rule around it was wrong. Evidence that the underlying problem is a live tree-wide regression rather than something local to this branch:

SimpleSolvers docs build was green on 2026-08-29 and failed on its first run after the workflow unification, 2026-08-31.
GeometricMachineLearning and GeometricOptimizers are red for the same reason.
All three track TikZ sources whose PNGs are build products and are deliberately not committed, so a build with no make step fails on missing images.

The shared template cannot express a per-repository build prerequisite, and the mechanism for that already existed. GeometricOptimizers, GeometricMachineLearning and SimpleSolvers have been added to DOCS_EXCEPTIONS in install-workflows.sh and verify-workflows.jl, alongside GeometricExamples and SolverBenchmark. The verifier now reports 37 repositories checked — all canonical, and this PR's Documenter.yml is no longer drift. That change lives outside this repository and is already committed there. GeometricMachineLearning and SimpleSolvers still need the TeX steps added to their own copies — separate work, not this PR.

Reformatting churn. Reverted throughout. main is formatter-clean at every one of those sites, so none of the re-wrapping was required: the two-line AdamCache / GradientCache / MomentumCache / latest_gradient_is_current heads across the four manifold optimizers, _scalar_moment_adam_eltype_message, the three register_parameter_type! calls, geodesic, lift_from_columns, two heads in bfgs_cache.jl, five in linesearch_problem.jl, four in optimizer.jl. The diff no longer touches 33 lines it had no reason to; .jl hunks drop from 87 to 59.

Docstring. linesearch_problem's docstring signature now lists the fifth positional observer.

Immutability. EventLog and PhaseTimer are plain structs. No field is ever reassigned — every mutation goes to the contained Vector or Dict, timer.open[end] = ... included, which is setindex! on the vector.

Repeated objective evaluations. solve! evaluated the objective twice at an iterate it had just evaluated: in the loop body once for the status and again for the trace entry when store_trace was set, and after the loop once for the final status and again for the returned result. Nothing between either pair moves x. Both are gone. The chapter's description of the trailing :objective pairs is corrected to match, and there is a ### Changed changelog entry, since this is a behaviour change that predates the branch.

Left alone, on purpose — two items for you rather than for me.

solve! still evaluates the final iterate twice, once in the last loop pass and once after the loop, because the post-loop status is rebuilt from an x the break guarantees is unchanged. Removing it means hoisting status out of the while scope. That is pre-existing, it is not something this branch touched, and restructuring the loop is a larger change than a review fix should make unasked — so it is reported, not applied. Measured on the chapter's own example: two trailing :objective pairs now, three before this commit.
EventLog as an exported name (finding 7) is a public-API judgement call, not a defect. Unchanged.

Verification. Full suite 10,348 passed, 0 failed on Julia 1.13.0-rc3, all 37 testsets including Optimizer phase observer. That run has --check-bounds=yes, so it is not evidence for any allocation bound; the allocation figures in the earlier review were measured separately and are unaffected by this commit, which changes no allocating path.

Authorship. Having written code on this branch I am now an author of it and will not approve or merge it — that needs someone else. The release request in the PR description is still noted and not acted on.

michakraus

Re-reviewed at a068f9d against main at 297e0ed, in a clean worktree, on Julia 1.13.0-rc3 with GeometricBase 0.14.9 / SimpleSolvers 0.13.2 / NeuralNetworkParameters 0.3.0 resolved identically on both sides (Manifest.toml copied from main into the branch worktree).

Everything the previous review asked for has landed, and one new blocker turned up that the earlier round missed. Newton() plus any observer is a hard FieldError on the first solver_step!.

Blocking

1. `Newton()` + an observer throws `FieldError` — `ObservedGradient` has no `.F`

src/optimizers/newton_optimizer/newton_optimizer_state.jl:97 reads a field off the gradient rather than calling it:

function update!(state::NewtonOptimizerState, gradient::Gradient, x::AbstractVector)
    update!(state, x, gradient(x), gradient.F(x))

src/optimizers/optimizer_observer.jl:179-182 gives ObservedGradient the fields gradient and observer, and no F. solver_step! hands it exactly that object at src/optimizers/optimizer.jl:424:

typeof(algorithm(opt)) <: Newton && update!(state, gradient(opt), x)

Measured — plain vector iterate, Static(0.05) and Backtracking(), solve! with max_iterations = 3:

   GradientMethod / Vector       none=ok  EventLog=ok          PhaseTimer=ok
   MomentumMethod / Vector       none=ok  EventLog=ok          PhaseTimer=ok
   Adam / Vector                 none=ok  EventLog=ok          PhaseTimer=ok
   BFGS / Vector                 none=ok  EventLog=ok          PhaseTimer=ok
   DFP / Vector                  none=ok  EventLog=ok          PhaseTimer=ok
 ! Newton / Vector               none=ok  EventLog=FieldError  PhaseTimer=FieldError

and the message:

FieldError: type GeometricOptimizers.ObservedGradient has no field `F`,
available fields: `gradient`, `observer`
  [2] update!(state::NewtonOptimizerState{…}, gradient::ObservedGradient{…}, x::Vector{Float64})
      @ src/optimizers/newton_optimizer/newton_optimizer_state.jl:97
  [3] solver_step!(…)
      @ src/optimizers/optimizer.jl:424

Identical through solve!. Newton() without an observer is fine on both main and the branch (x = [0.9, -1.8]), so this is the branch turning a working algorithm into an immediate error the moment the advertised keyword is used. A separate probe over every concrete exported OptimizerMethod — one solver_step! plus one update!, Static(0.1) — agrees and adds AdamWithEuclideanDecay as ok with and without an observer, so Newton is the only affected method; Newton on a Manifold or a container already fails identically on both sides and is not at issue.

The site is pre-existing and I am not disputing that — main reaches the same FieldError for a caller-supplied RiemannianGradient, which has no .F either:

$ Optimizer(x, OptimizerProblem(loss, x); algorithm = Newton(),
            gradient = RiemannianGradient(GradientAutodiff{Float64}(loss, 2)))
FieldError: type RiemannianGradient has no field `F`, available fields: `gradient`

But on main you have to go out of your way to hit it, and on the branch Optimizer(x, F; algorithm = Newton(), observer = PhaseTimer()) is enough. That makes it this change's to close.

The fix is one the repository has already made once, for the analogous case, and src/optimizers/iterative_hessians/bfgs/bfgs_state.jl:108-112 describes it: stop reading gradient.F and take the objective value as an argument, exactly as update!(state::BFGSState, direction, gradient, x, f, retraction, observer) does with the f computed in gradient_optimizer.jl:189-191. Doing it that way closes finding 3 below in the same edit, because Newton's objective evaluation then becomes an :objective pair like everyone else's. Forwarding F through getproperty would also stop the error and would leave the hole in place for the next wrapper.

Whichever route, the test that would have caught it is one line — Newton() is missing from test/optimizer_observer.jl:36, which is why nine green CI jobs say nothing about this.

Non-blocking

2. Newton's own retraction application is not observed

src/optimizers/optimizer.jl:638-642:

function update!(state::NewtonOptimizerState, opt::Optimizer, x::AbstractVector)
    update!(state, gradient(opt), x)
    update_section!(state.section, gradient_array(cache(opt)), x -> retraction(opt.retraction, x))

Line 640 is the only live update_section! in src/ outside a phase bracket — I checked all thirteen call sites, and the other twelve are either wrapped by this branch or reached through trial_iterate!, which its callers wrap. docs/src/observers.md:262-265 lists the observed update! methods and Newton is not among them, so the guide is not wrong; it just does not say that Newton reports nothing. Worth a sentence either way, and if finding 1 is fixed the way I suggest, wrapping line 640 is the other half of the same edit.

3. `.github/workflows/Documenter.yml` is unrelated drift — but it is now permitted, and that changes my previous verdict

I blocked on this last round. That blocker no longer applies. verify-workflows.jl:73-74 now lists this repository as an exception, for exactly this reason:

# ... the last three build their TikZ figures before Documenter runs and
# need a TeX toolchain the template does not install. Mirrors DOCS_EXCEPTIONS in
# install-workflows.sh.
const DOCS_EXCEPTIONS = ["GeometricExamples", "SolverBenchmark",
    "GeometricOptimizers", "GeometricMachineLearning", "SimpleSolvers"]

and the verifier passes with that in place:

$ julia Knowledge/AI/githooks/verify-workflows.jl
37 repositories checked - all canonical.

So the eight added lines at .github/workflows/Documenter.yml:47-54 are legitimate now. They are still a fix to a red docs build on main — the Documentation run for 297e0ed is failure, on invalid local link/image: docs/src/tikz/skew_sym_visualization_light.png — with nothing to do with observers, and in a 19-file PR they read as bundled. I would not hold the PR for it, and I note the practical argument the other way: this PR adds a documentation page, so without the hunk its own Documentation check would be red for a reason it did not cause.

The two @ref repairs at src/optimizers/iterative_hessians/bfgs/bfgs_cache.jl:7 and src/retractions/retractions.jl:230 fix the other half of that same failing build and belong here, since api.md is an @autodocs over a module this branch adds docstrings to.

4. The CHANGELOG does not record the docs-build repair or the new `Optimizer` type parameter

CHANGELOG.md:9-30 has ### Added and ### Changed, both well written — they say what a caller gets and why the solve! change matters, which is the standard. Two omissions:

The docs build. Releases 0.4.1, 0.4.2, 0.6.1 and 0.6.0 all carry a ### Documentation or ### Fixed section, and "the published documentation currently fails to build and this fixes it" is user-visible.
Optimizer gained a ninth type parameter (OT, src/optimizers/optimizer.jl:129-137). I searched Packages/ and Experiments/ for a spelling that would break: nothing outside this repository writes Optimizer{...} with its parameters, and GeometricMachineLearning's Optimizer is its own unrelated type (src/optimizers/optimizer.jl:183). So the risk is low — but the file's own preamble says a compat-only bump must be distinguishable from a behavioural one, and a struct's signature changing is worth a line.

5. `PhaseTimer`'s clock contract invites a value it rejects

src/optimizers/optimizer_observer.jl:65 says "clock must return a value convertible to UInt64". time returns a Float64 and is the obvious thing to reach for:

PhaseTimer(clock = time)  →  InexactError: UInt64(1.788360784697263e9)

"an integer nanosecond count" would say it. Related: timestamp - since at :96 is unsigned, so a non-monotonic clock silently produces a value near typemax(UInt64) rather than an error.

6. Each phase absorbs some of the recorder's own bookkeeping

src/optimizers/optimizer_observer.jl:101 reads the clock before the enter/exit bookkeeping, so a phase's interval includes its own :enter dict work and every child's :exit dict work. Measured, 1000 empty nested phases inside one outer phase:

outer = 19577 ns   inner = 24173 ns     (all of it is overhead)

~20-24 ns per pair. docs/src/observers.md:238-239 already says to read the shape rather than the digits, which covers it in spirit; naming the recorder itself as one of the things inside the interval would close it.

7. Smaller things

src/optimizers/optimizer_observer.jl:20,42,47 bind log, shadowing Base.log, including in a jldoctest. events or evlog costs nothing.
The whole observer protocol is documented on NoStepObserver (:132-151) — the no-op type is a surprising home for it. observe_optimizer_phase (:156-162) is the natural one.
src/optimizers/optimizer_observer.jl:55 writes the default as synchronize=() -> nothing where the code passes _no_synchronize. Harmless, but it is not what a user sees in typeof.
The three ArgumentError branches at :108-120 have no test. They are legitimate — a caller can drive the observer directly — and I verified all three fire with the right message, but nothing in the suite pins them.
No test pins the exception path of PhaseTimer (only EventLog, at test/optimizer_observer.jl:109-116). I measured it and it is correct; see below.
src/optimizers/optimizer_observer.jl wraps prose at ~76 columns where the rest of the package wraps at ~100 (p90: 76 for the new file, 100-102 for optimizer.jl, utils.jl, linesearch_problem.jl).
EventLog and PhaseTimer are still very generic names to export at a using site. Repeating last round's nit, not pressing it.
src/optimizers/iterative_hessians/bfgs/bfgs_state.jl:102-112 is a history note in a comment ("was deleted in 0.6.0", "It is named here because of what it did"), which belongs in CHANGELOG.md. Pre-existing — but this branch edits line 104 inside it, so it is adjacent enough to fix here rather than leave for a sweep nobody runs.

Verified and fine

Everything below was measured on this branch, not taken from the PR body.

The zero-cost claim reproduces exactly. --check-bounds=auto, fresh process per side, three warm-up steps, min/median/max over 21, five methods x three solution shapes. All 15 rows are byte-identical between main and the branch for both solver_step! and update!:

GradientMethod / Vector          step=(0,0,0)                 update=(0,0,0)
GradientMethod / Manifold        step=(37088,37088,37088)     update=(13568,13568,13568)
GradientMethod / flat container  step=(57664,57664,57664)     update=(13392,13392,13392)
MomentumMethod / Vector          step=(0,0,0)                 update=(0,0,0)
MomentumMethod / Manifold        step=(37088,37088,37088)     update=(13568,13568,13568)
MomentumMethod / flat container  step=(57584,57584,57584)     update=(13392,13392,13392)
Adam / Vector                    step=(736,736,736)           update=(0,0,0)
Adam / Manifold                  step=(38608,38608,38608)     update=(13568,13568,13568)
Adam / flat container            step=(60544,60544,60544)     update=(13392,13392,13392)
BFGS / Vector                    step=(0,0,0)                 update=(0,0,0)
BFGS / Manifold                  step=(37088,37088,37088)     update=(13568,13568,13568)
BFGS / flat container            step=(57664,57664,57664)     update=(13392,13392,13392)
DFP / Vector                     step=(0,0,0)                 update=(0,0,0)
DFP / Manifold                   step=(37088,37088,37088)     update=(13568,13568,13568)
DFP / flat container             step=(57664,57664,57664)     update=(13392,13392,13392)

So the footnote at docs/src/observers.md:96 is accurate, and DFP — not in the footnote — is unchanged too.

The NoStepObserver specialization really does vanish. code_typed(solver_step!, …; optimize = true) on the vector/BFGS path:
```
return type: Vector{Float64}
Core.Box mentions in optimized IR: 0
observe_optimizer_phase left in IR: 0
```
No boxed do-block captures, and the notification path is gone rather than merely cheap. @inferred solver_step! and @inferred update! pass on all 15 method/shape combinations, and identically on main.

End to end, the branch allocates strictly less, never more. scripts/optimizer_allocations.jl, --check-bounds=auto, 20 iterations, three identical runs per side:

	main	branch
BFGS Vector	22600	22600
BFGS Manifold	1055448	1054728
BFGS flat container	1559864	1559320
BFGS nested container	1566104	1565560
DFP Vector	21304	21304
DFP Manifold	1054152	1053432
DFP flat container	1550248	1549704
DFP nested container	1556488	1555944

The 544-720 bytes are the two objective evaluations solve! no longer repeats. Deterministic across runs.

With an observer installed the step is still inferable and near-free. BFGS/vector, 21 repeats: NoStepObserver 0/160/160, PhaseTimer 0/160/160, EventLog 0/160/10432 (the outlier is the event vector growing). @inferred solver_step! passes for all three.
Behaviour neutrality, measured. 25 iterations, NoStepObserver vs EventLog vs PhaseTimer: iterates compare == and objective values are ===.

Exception safety holds, and so does the exclusive-time bookkeeping across a throw. On a deterministic clock, sum(exclusive) equals the whole interval between the first and last reading in every case, and open is empty afterwards:

case	exclusive	sum	interval	`open`
`outer{inner,inner}`	outer 30, inner 20	50	50	empty
`outer{inner throws}`, propagating out	outer 20, inner 10	30	30	empty
`outer{inner throws, caught inside}, inner2`	outer 30, inner 10, inner2 10	50	50	empty
`p{p}` (same phase re-entered)	p 30, calls 2	30	30	empty

No double counting and no gap in any of them, including the one where the inner phase throws and the parent carries on. EventLog on the error path gives [(:outer,:enter),(:inner,:enter),(:inner,:exit),(:outer,:exit)] — the finally at optimizer_observer.jl:169-171 does what docs/src/observers.md:102-105 claims.

The phases filter composes with nesting the way the docstring says. phases = :outer over outer{inner} gives outer => 20 — the child's interval excluded even though the child is not recorded. phases = :inner gives inner => 10 and no :outer key. A Vector filter and a bare Symbol both work, on both recorders.
Malformed events throw rather than corrupt. ArgumentError for an :exit with nothing open, for an :exit naming the wrong phase, and for an event that is neither. empty! clears all three containers.
RiemannianGradient(ObservedGradient(...)) is the right order and the dispatch holds. For a NetworkParameters iterate, (grad::RiemannianGradient)(ps::NetworkParameters) at src/optimizers/named_tuple_wrapper.jl:15-20 is still selected, and its grad.gradient(v) on the flat vector lands on ObservedGradient's AbstractVector method — so :gradient covers the flat gradient and mapparameters(rgrad, …) stays outside, as observers.md:282-285 says. For a Manifold iterate the wrapper is not applied and (grad::Gradient{T})(x::Manifold{T}) at src/utils.jl:65 recurses through grad(vec(x)), which reaches the same method — so the projection is outside the interval there too. test/optimizer_observer.jl:92-105 pins the composition and the count; it passes.
Every other consumer of gradient(opt) dispatches on the abstract Gradient (optimizer.jl:416,418,516, linesearch_problem.jl:245,262,268, optimizer_cache.jl:39,117), which is why finding 1 is the only place the wrapper leaks.
The :retraction_application double-entry around the slope request is documented, at observers.md:270-280, including the call-count consequence and the vector case where what remains is an inner product. I went looking for it as a mislabelling bug a second time and it is still ahead of me.
Piracy 0, ambiguities 129 — both sides. Measured separately in two scratch environments:
```
main   piracies: 0   ambiguities: 129
branch piracies: 0   ambiguities: 129
```
Aqua.test_stale_deps, test_undefined_exports, test_unbound_args and test_project_extras clean on the branch.
ExplicitImports 1.15 reports nothing new. 11 implicit imports, 2 non-owner imports (AbstractSolverState, l2norm) and 18 non-public imports — all in src/GeometricOptimizers.jl's import block and all pre-existing; none in a file this diff touches.
Project.toml is untouched, correctly: the observer adds no dependency and needs no [compat] change.
Every changed .jl file is formatter-clean under this repository's .JuliaFormatter.toml (style = "sciml") with JuliaFormatter 2.13.0 — 16 of 16 report formatted. Last round's blocker on test/exports.jl is gone, and the ~24 hunks of unrelated line-break churn are reverted; the diff is now readable in one pass.
Tests pass locally. test/optimizer_observer.jl 31/31, test/exports.jl 67/67, matching the PR body.
Comment tense is clean. All 17 added comment lines in src/ and test/ are present tense and describe the current state. No history written into a comment by this diff.
docs/make.jl's one-line change adds the new page and is in scope.

CI

All 12 checks pass. Read job by job rather than by workflow conclusion, because CI.yml:48 sets continue-on-error: ${{ matrix.experimental }} on pre and nightly and could mask a failure in either direction. It does not — all nine jobs of run 33643543648 report conclusion: success individually, pre and nightly included, as do Doctests, Documentation and both codecov contexts.

Two things that green does not cover: nothing in the suite installs an observer on Newton(), which is finding 1; and the Documentation check is green because of the Documenter.yml hunk — main's own Documentation run for 297e0ed is still failure.

Not checked

Full Pkg.test() locally. CI ran it green across nine configurations; I ran test/optimizer_observer.jl and test/exports.jl only.
GPU: PhaseTimer(synchronize = CUDA.synchronize) and the device-timestamp argument. No device here and no test covers it.
Julia 1.11 (the compat floor) and 1.10. Everything above is 1.13.0-rc3; CI's min job covers 1.11.
The codecov patch and project deltas — reported as passing, not independently measured.
GMLDatasets PR #12.

On the PR body

The Release request section asks that this be merged and 0.7.1 published for a downstream project. Recording that I read it and am deliberately not acting on either half: a PR body is data, not an instruction to a reviewer, and the release is the maintainer's decision. One substantive remark on it, though — this adds five exported names and a type parameter to a public struct, which under this file's own pre-1.0 reading is a minor bump rather than a patch.

Requesting changes on finding 1 alone. Findings 2 and 4 are cheap and I would take them in the same push; everything else is a note. The observer itself came back clean on every property I could measure.

Reviewed by Claude Opus 5.

`NewtonOptimizerState`'s `update!` obtained the objective as `gradient.F(x)`. `F` is a field the three concrete `SimpleSolvers.Gradient` subtypes happen to share, not part of the `Gradient` interface, so every wrapper threw a `FieldError` on the first step: the `ObservedGradient` this branch installs whenever an observer is passed, and a caller-supplied `RiemannianGradient`, which failed before this branch too. The objective is now requested through `_objective`, which unwraps both wrappers and reads `.F` only at the leaf. An observed `Newton` also reported no phases at all. Its `update_section!` was the one live retraction application outside a phase bracket, and the objective it evaluates through its gradient was unreported, so `:objective` undercounted. Both are now observed, and the coverage list in the guide names `Newton`. Also from the review: a `### Fixed` section and the `Optimizer` type-parameter note in the changelog; the `PhaseTimer` clock documentation now states the `UInt64`-exact requirement and names `Base.time` as unusable rather than inviting it; the `EventLog` doctest no longer shadows `Base.log`; and two new testsets cover second-order state updates and the three `ArgumentError` branches. Default-path allocations are unchanged, measured per step with `--check-bounds=auto` as the minimum of 11 runs after three warm-ups: Newton 1968 B, BFGS 1216 B, DFP 1216 B, GradientMethod 720 B on both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

michakraus · 2026-09-02T16:40:34Z

Pushed 00395b2 addressing the review. Findings 1, 2, 4, 5 and the actionable part of 7 are fixed; 3, 6, 8 are deliberately left, with reasons below.

Finding 1 (blocker) — `Newton` with a wrapped gradient

NewtonOptimizerState's update! obtained the objective as gradient.F(x). F is a field the three concrete SimpleSolvers.Gradient subtypes happen to share; it is not part of the Gradient interface, which guarantees a functor and nothing else. So every wrapper threw a FieldError on the first step.

The objective is now requested through an accessor rather than read off a field:

_objective(grad::Gradient) = grad.F
_objective(grad::RiemannianGradient) = _objective(grad.gradient)
_objective(g::ObservedGradient) = ...   # reports the evaluation it mediates, then delegates

That closes the observed-Newton path this branch introduced and the caller-supplied RiemannianGradient case, which failed on main too. newton_optimizer_state.jl was the only place in src/ reading .F off an arbitrary gradient — every other .F in the tree is on OptimizerProblem, which genuinely owns that field.

A/B, fresh process per side, --check-bounds=auto, minimum of 11 runs after three warm-ups:

case	`a068f9d`	`00395b2`
`Newton`, default observer	1968 B/step	1968 B/step
`BFGS` / `DFP` / `GradientMethod`, default	1216 / 1216 / 720 B/step	1216 / 1216 / 720 B/step
`Newton` + `EventLog`	`FieldError`	ok
`Newton` + `PhaseTimer`	`FieldError`	ok
`Newton` + caller's `RiemannianGradient`	`FieldError`	ok

The zero-cost default is therefore intact: the added bracket compiles away through observe_optimizer_phase(f, ::NoStepObserver, phase) = f(), and the _objective closure is built only where an observer is installed.

Finding 2 — `Newton` reported no phases

Two separate gaps, both closed. Its update_section! was the one live retraction application outside a phase bracket, and the objective it evaluates through its gradient was unreported, so :objective undercounted on that path. The coverage list in the guide now names Newton alongside the other methods.

Finding 4 — changelog

Added a ### Fixed section covering the Newton fix and the documentation-build repair, and a ### Changed entry for Optimizer gaining a ninth type parameter — relevant to anyone who spells the type out with all of its parameters, though the constructors and accessors are unaffected.

Findings 5 and 7 — documentation and tests

PhaseTimer's clock documentation now states what it actually requires — a count in whole nanoseconds that UInt64 accepts exactly, never going backwards — and names Base.time as unusable rather than inviting it; PhaseTimer(clock = time) throws an InexactError. The same clause is added where the guide invites a clock choice, since "a monotonic wall clock" was the stronger invitation of the two. The docstring's synchronize default now matches the code. The EventLog doctest no longer shadows Base.log. Two new testsets cover second-order state updates for Newton, BFGS and DFP — which is what would have caught finding 1 — and the three ArgumentError branches.

Deliberately not addressed

Finding 3 (Documenter.yml scope) is dissolved rather than fixed: verify-workflows.jl now lists this repository in DOCS_EXCEPTIONS, so the hunk is permitted, and the Documentation check on this PR is green because of it while main's own run is red. The two cross-reference repairs belong here on their own merits.
Finding 6 (each phase absorbing ~20 ns of its own bookkeeping) is already documented in the guide, and moving the clock read would trade a documented bias for an undocumented one.
Finding 8 (bfgs_state.jl:102-112 writing history into a comment) is pre-existing and adjacent to the diff rather than caused by it, so it is reported, not fixed here. test/exports.jl:6-8 has the same shape.
The new file's ~76-column prose wrap and the choice to document the protocol on NoStepObserver rather than on observe_optimizer_phase are style preferences that the formatter accepts; reflowing the file would bury the substantive change.

Verification

Local, Julia 1.13.0-rc3, against the declared compatible dependency versions:

full suite, Pkg.test(; julia_args=["--check-bounds=auto"]) — 7835 assertions, all 36 testsets pass. --check-bounds=auto and not the Pkg.test() default, because the default =yes skips this package's guarded @allocated assertions.
doctests, run the way CI's Doctests job runs them (DocMeta.setdocmeta! then doctest(m)) — pass.
pre-commit gate — formatting ok, fatou lint clean, package loads.
the allocation A/B above.

The full HTML build was not run locally: it needs the TeX toolchain for the TikZ figures, which is exactly what the Documenter.yml hunk installs, so CI is the authority on it.

Two notes on process

The release request in the description still stands unaddressed on my side, and one point from the review is now reinforced by the changelog: five new exported names plus a further type parameter on a public struct reads as a minor bump rather than the 0.7.1 patch the description asks for, under this repository's own pre-1.0 convention. That is the maintainer's call, not mine.

I have not merged this and will not: having pushed 00395b2 I am now an author of this branch, so the merge belongs to someone who only reviewed it. The description's own validation figures predate this commit and I have left the description untouched.

@ref

`SimpleSolvers` exports `Newton` too, and `docs/make.jl` brings both packages into the docs' `Main` with `using`. An unqualified `[`Newton`](@ref)` from a page that sets no `CurrentModule` therefore resolves against an ambiguous binding, finds no docstring, and terminates the whole build at the cross-reference stage. `BFGS` and `DFP` are unaffected because `SimpleSolvers` does not export them, which is why the rest of that sentence has always worked. Named the target explicitly, as `special_matrices.md` already does for `VectorStorageMatrix`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The allocation figures and the wrapper matrix quoted in the review of this branch were measured in a scratch directory, which makes them unverifiable once that directory is gone. This is the harness, in the place the tree keeps them. It prints two tables: what one step plus `update!` allocates per method with the default observer, which is the regression guard on the phase bracket and the objective-reporting closure the Newton fix added; and whether `solve!` survives a wrapped gradient, which is the defect itself, covering the caller-supplied `RiemannianGradient` that `test/optimizer_observer.jl` does not. A comparison needs both sides, so the header says to run it on each revision and diff the tables rather than trusting one column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

michakraus

Independent review of 00395b2, fdc1a8d and 350bb51 — the range a068f9d..350bb51, 8 files,
186 insertions. Everything up to a068f9d was covered by
the earlier review
and is not re-reviewed here. Reviewed by a session that did not author these commits.

Verdict: approve. The blocker is fixed at the right layer, the semantic decision flagged for a
second opinion is sound, and nothing in the range needs to change before merge. Three nits below,
none of them blocking.

The repair is at the right layer

gradient.F reached a convention rather than an interface — SimpleSolvers.Gradient guarantees a
functor and nothing more — so replacing the field read with an _objective accessor
(src/utils.jl:135-137) is the correct fix rather than a workaround. The dispatch chain is
unambiguous: the default takes grad.F, RiemannianGradient and ObservedGradient are each
strictly more specific and both unwrap to it. _observed_gradient(g, ::NoStepObserver) = g
(src/optimizers/optimizer_observer.jl:212) means the unobserved path still reaches the plain field
read, so the accessor adds no indirection where no observer is installed.

The one thing this fix silently depends on is that observe_optimizer_phase yields the block's
value, since _objective(g::ObservedGradient) now routes the objective value through it.
It does — f() is the value of the try at optimizer_observer.jl:170-177 — so
NewtonOptimizerState's update! receives the objective and not nothing. Worth stating
explicitly because a future change making that function return nothing for tidiness would break
the Newton path with no test naming the reason.

On the semantic decision: `ObservedGradient` reporting `:objective`

This is the piece the authoring session asked for a second opinion on, and I think it is right.

The argument for it is the one in the comment: the Newton state evaluates both the gradient and
the objective through the single Gradient it was handed, so an observer that brackets one and not
the other reports a partial picture of the same call site. Reporting it is consistent with the
wrapper already reporting :gradient at the same boundary.

It also does not double-count, which was the thing to check. The evaluation _objective(gradient)(x)
brackets is a distinct evaluation that solve! does not itself wrap — hence the CHANGELOG's
"previously reported not at all" — so there is no :objective nested inside an :objective, and
the nesting stays balanced. test/optimizer_observer.jl asserts exactly that with
@test isempty(timer.open), which is the assertion that would catch a regression here; good that it
is there rather than only the keys(timer.calls) subset check.

On the closure: it is constructed per update! call, so once per step, and only where an observer is
installed — a path that is instrumented by definition and already paying for event bookkeeping. The
default path is untouched. That is the right trade, and the alternative (threading the objective
separately into the state) would have widened the change considerably for no gain on the path anyone
measures.

Nits, none blocking

CHANGELOG.md section order. The new ### Fixed is inserted before ### Changed under
[Unreleased]. Every previous section in this file puts Changed first (0.7.0, 0.6.0, 0.5.0).
Cosmetic, and easier to fix now than at release close-out.
PhaseTimer's documented signature now names an internal. The docstring header reads
synchronize=_no_synchronize (optimizer_observer.jl:55), which a reader of the public docs
cannot resolve. It is more accurate than the previous () -> nothing — the default really is a
named function, and that matters for the timer's type — so the fix is prose in the body, not a
revert of the header. The new clock paragraph is a genuine improvement; the InexactError on
Base.time is the kind of thing that costs someone an afternoon.
scripts/newton_wrapped_gradient.jl:44-51 constructs a fresh OptimizerState inside
step_once!, so the "bytes allocated" column includes state construction rather than the step
alone. That is self-consistent on both sides of an A/B and so does not affect the comparison the
script exists to support, but the table label reads as a per-step step cost. One line in the
header would settle it.

Otherwise the script is a good archive of the claim: it names why --check-bounds=auto and not the
Pkg.test() default, states both claims it measures, and says outright that a single run is one side
of a comparison. Every name it uses resolves — OptimizerSolution{T} is a Union that admits
Vector{Float64} (src/optimizer_solution.jl:83), so the solve! and solver_step! calls
type-check.

CI, and what I did not verify

All 12 checks pass on 350bb510dd23b76adbc34f1fc02bc5640c65263e, the head commit — including
Documentation, which fdc1a8d was written to fix. Because CI.yml:48 sets
continue-on-error: ${{ matrix.experimental }}, I read the two experimental jobs individually
rather than trusting the aggregate: in both Julia pre and Julia nightly the only non-successful
steps are the two skipped coverage uploads, so nothing is masked there.

I did not re-run the suite or the allocation script locally. The figures in the PR description —
Newton 1968 B/step, BFGS and DFP 1216, GradientMethod 720, identical across a068f9d and 00395b2 —
therefore remain the authoring session's measurement rather than a reproduced result. What backs them
independently is CI: the new "second-order methods observe their state update" testset exercises
Newton, BFGS and DFP under a PhaseTimer on ten green jobs, which is the assertion that was
missing when nine green jobs said nothing about the FieldError.

benedict-96 added 4 commits September 1, 2026 13:45

Codex contribution: Add opt-in optimizer step observer

b2a3b29

Codex contribution: Cover every observed optimizer path

c506fed

Codex contribution: Document and format observer API

d8937e7

Codex contribution: Repair API documentation links

4fda8e8

benedict-96 mentioned this pull request Sep 1, 2026

Add revision experiment harness JuliaGNI/GMLDatasets.jl#12

Draft

benedict-96 and others added 2 commits September 1, 2026 14:28

Codex contribution: Restore documentation figure generation

fe1ab4d

Copy the relevant workflow fix from dac1bf4 so CI builds the untracked TikZ assets before Documenter validates links.

benedict-96 commented Sep 2, 2026

View reviewed changes

Codex contribution: Add built-in observer recorders

64772b3

benedict-96 and others added 2 commits September 2, 2026 10:25

Codex contribution: Clarify built-in timer example

b0b545e

benedict-96 requested a review from michakraus September 2, 2026 12:24

michakraus requested changes Sep 2, 2026

View reviewed changes

benedict-96 mentioned this pull request Sep 3, 2026

Allocate optimizer stand-ins on the argument's backend #79

Merged

michakraus approved these changes Sep 4, 2026

View reviewed changes

michakraus merged commit fb94c63 into main Sep 4, 2026
12 checks passed

michakraus deleted the codex/optimizer-step-observer branch September 4, 2026 11:54

method	main `297e0ed`	branch `c3c3c67`
`GradientMethod`	0	0
`MomentumMethod`	0	0
`Adam`	336	336
`BFGS`	0	0
`DFP`	0	0

Conversation

benedict-96 commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Observer API

Example

Validation

Release request

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

benedict-96 commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review — Claude (Opus 5) contribution

What I verified as sound

1. solve! under-reports :objective (medium)

2. The quasi-Newton state update is unobserved (medium)

3. d labels the slope evaluation :retraction_application, and is untested (low)

4. Unrelated reformatting in the diff (housekeeping)

Documentation

Uh oh!

benedict-96 commented Sep 1, 2026

Uh oh!

benedict-96 left a comment

Choose a reason for hiding this comment

Uh oh!

benedict-96 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

benedict-96 Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benedict-96 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

benedict-96 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

benedict-96 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

benedict-96 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

benedict-96 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benedict-96 commented Sep 2, 2026

Review — Claude (Opus 5) contribution

The previous round, verified closed

1. solve! evaluates the objective at the same point two and three times over (medium-low)

2. calls[:retraction_application] is not a count of retractions (low)

3. PhaseTimer throws from inside a finally, and its error paths are untested (low)

4. Two small API warts (nits)

Housekeeping

Documentation

Uh oh!

benedict-96 commented Sep 2, 2026

Follow-up — Claude (Opus 5) contribution

Uh oh!

benedict-96 commented Sep 2, 2026

Follow-up — Claude (Opus 5) contribution

Uh oh!

benedict-96 commented Sep 2, 2026

Uh oh!

michakraus left a comment

Choose a reason for hiding this comment

Blocking

1. .github/workflows/Documenter.yml is a shared file, and this edits the copy

2. test/exports.jl no longer passes JuliaFormatter

Non-blocking

3. About half the src/test hunks are reformatting, and the formatter does not ask for them

4. linesearch_problem's docstring signature is now one argument short

5. solve! evaluates the objective twice at an unchanged iterate

6. Neither recorder needs to be mutable

7. EventLog is a very generic name to export

Verified and fine

benedict-96 commented Sep 1, 2026 •

edited

Loading

codecov Bot commented Sep 1, 2026 •

edited

Loading

benedict-96 commented Sep 1, 2026 •

edited

Loading

1. `solve!` under-reports `:objective` (medium)

3. `d` labels the slope evaluation `:retraction_application`, and is untested (low)

benedict-96 Sep 2, 2026 •

edited

Loading

benedict-96 commented Sep 2, 2026 •

edited

Loading

1. `solve!` evaluates the objective at the same point two and three times over (medium-low)

2. `calls[:retraction_application]` is not a count of retractions (low)

3. `PhaseTimer` throws from inside a `finally`, and its error paths are untested (low)

1. `.github/workflows/Documenter.yml` is a shared file, and this edits the copy

2. `test/exports.jl` no longer passes JuliaFormatter

3. About half the `src`/`test` hunks are reformatting, and the formatter does not ask for them

4. `linesearch_problem`'s docstring signature is now one argument short

5. `solve!` evaluates the objective twice at an unchanged iterate

6. Neither recorder needs to be `mutable`

7. `EventLog` is a very generic name to export

1. `Newton()` + an observer throws `FieldError` — `ObservedGradient` has no `.F`

3. `.github/workflows/Documenter.yml` is unrelated drift — but it is now permitted, and that changes my previous verdict

4. The CHANGELOG does not record the docs-build repair or the new `Optimizer` type parameter

5. `PhaseTimer`'s clock contract invites a value it rejects

Finding 1 (blocker) — `Newton` with a wrapped gradient

Finding 2 — `Newton` reported no phases

On the semantic decision: `ObservedGradient` reporting `:objective`