Skip to content

Avoid discarded objective evaluations in OptimizationOptimisers#1268

Merged
ChrisRackauckas merged 3 commits into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-optimisers-extra-objective-evaluation
Jul 22, 2026
Merged

Avoid discarded objective evaluations in OptimizationOptimisers#1268
ChrisRackauckas merged 3 commits into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-optimisers-extra-objective-evaluation

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Jul 15, 2026

Copy link
Copy Markdown
Member

Summary

  • Remove the discarded pre-loop objective call from OptimizationOptimisers.
  • Initialize the eventual objective result with an infinity converted to eltype(real(cache.u0)), avoiding an undefined/nullable placeholder and a hard-coded Float64.
  • Evaluate the initial objective only when the optimizer loop completes zero iterations, and count that fallback evaluation.
  • Add public-API regressions for ordinary iterations, the rounded-zero fallback, and a genuine infinite Float32 objective.

OptimizationOptimisers 0.3.19 initialized the eventual objective result by evaluating cache.f before the loop. Normal iterations overwrite that value, so the call was unreported work and advanced global RNG state for stochastic objectives before optimization began.

The typed Inf is only an initialization value. The fallback is keyed to iszero(iterations), not to the value of x; therefore an objective that genuinely returns infinity after an iteration is not evaluated a second time. Constructing it with convert(eltype(real(cache.u0)), Inf) yields Inf32 for Float32 parameters and a real Float32 infinity for ComplexF32 parameters.

Local verification

  • Julia 1.12.6, GROUP=OptimizationOptimisers: Core — 39 passed, 1 pre-existing broken; both OptimizationOptimisers and root Optimization test harnesses passed.
  • Julia 1.12.6, GROUP=OptimizationOptimisers_QA: 18 passed, 1 failed, 1 pre-existing broken. The sole failure is the public-API rendered-docs check reporting approximately 300 re-exported names; the identical failure reproduces on detached clean origin/master at 34a8fd5c.
  • The master failure is tracked in Optimization.jl #1278. Its exact dependency boundary is SciMLTesting 2.2.0 (passes 18/1 broken) to 2.3.0 (fails 18/1/1), introduced by SciMLTesting commit 044d89a. SciMLTesting #27 reduces the false positives from 300 to 9 inherited constants/enum values but does not yet fully fix the check.
  • Runic over every tracked Julia file: passed with no formatting diff.
  • git diff --check: passed.
  • Direct scalar probe: Float32 → Float32 Inf, Float64 → Float64 Inf, and ComplexF32 → Float32 Inf.

The regression uses OptimizationFunction and OptimizationProblem from their declaring public module, SciMLBase, and solve from CommonSolve. No assertion, tolerance, or expected-broken marker was loosened or disabled.

Please ignore this draft until it has been reviewed by @ChrisRackauckas.

Copy link
Copy Markdown
Member Author

Audit scratchpad for commit 1e488d74783dcafea92bce24fabf42e7fa54cbf3:

  • Provenance: Optimization PR Add compat bounds, ignore certain piracies, fix Aqua QA sublibrary tests #1230 / commit 060b812e4835ac4753f62f319d021aa4b0a0a50a, released in OptimizationOptimisers 0.3.19, added the discarded pre-loop objective call to initialize x for JET.
  • Exact regression counts after this patch: three ordinary iterations perform 0 standalone objective calls and 3 combined objective/gradient calls; a positive fractional maxiters that rounds to zero performs 1 fallback objective call, 0 gradient calls, and reports 1 function evaluation.
  • Julia 1.10 Core (33 pass, 1 pre-existing broken), Julia 1.10 QA/JET (16 pass, 1 pre-existing broken), root GROUP=OptimizationOptimisers, Julia 1.11 Core (33 pass, 1 pre-existing broken), and repository-wide Runic all passed locally.
  • Clean NeuralPDE master reproduced the target NNODE failure at 5/6. With this commit developed, AdaptiveLoss passed 12/12 and full NNODE passed 47/47.
  • The branch is one commit on current upstream be6fbd279a5e3b0d7cc0b9a2639f4fd64ff54c32; the fork's master matches upstream and the post-creation behind check is empty.

No test was skipped, silenced, loosened, or changed to match buggy output.

Copy link
Copy Markdown
Member Author

Scheduled CI checkpoint (2026-07-15 10:18 EDT):

  • The matrix is complete: 48 checks succeeded, 2 failed, and 3 were skipped.
  • All root package lanes are green: Core on LTS/current/prerelease, AD on LTS/current, and QA.
  • OptimizationOptimisers Core on LTS/current/prerelease and QA are green.
  • Both relevant NeuralPDE downstream groups are green: NNPDE1 in 1h17m14s and NNPDE2 in 50m29s. DiffEqFlux downstream is also green.
  • The two red checks are independently classified:
    • Documentation is an excluded standalone docs failure caused by nlopt.readthedocs.io returning HTTP 429 to linkcheck.
    • The excluded ModelingToolkit downstream job has GitHub's explicit “self-hosted runner lost communication” annotation; its test step remained marked in progress, with no completed failing assertion.

No observed package-test failure is attributable to this PR.

A separate NeuralLyapunov Benchmarking investigation should not be conflated with this result: its seeded stochastic policy tests remain context-sensitive, and a full run with this patch moves rather than robustly removes that package's accuracy failure. That will be tracked separately without changing assertions or tolerances.

@ChrisRackauckas-Claude
ChrisRackauckas-Claude force-pushed the agent/fix-optimisers-extra-objective-evaluation branch from 1e488d7 to 33d9282 Compare July 16, 2026 18:08
@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Post-rebase verification (2026-07-16): rebased commit 33d928249db93961ad7cedf3f4098210140baa18 onto current origin/master@876ddf60f8e13a68b84b84fbf3d8fac1d520bb0b. The exact Julia 1.12 GROUP=OptimizationOptimisers root Pkg.test() route exited 0; OptimizationOptimisers Core reported 33 pass plus 1 pre-existing broken, and the enclosing Optimization test invocation also passed. Generated manifests were removed, git diff --check passed, and the worktree was clean. Immediately before updating the PR, I fetched both remotes, verified that the base remained 876ddf60 and the feature branch remained at expected old head 1e488d74783dcafea92bce24fabf42e7fa54cbf3, then used an explicit force-with-lease for that exact old head.

Downstream audit note: a repeated full NeuralLyapunov Benchmarking run against this source passed 25/25 in 89m16.3s, but the same source failed the isolated ODESystem benchmark at 0.825 and an earlier full context failed the first benchmark at 0.62. I am therefore retaining this PR's deterministic evaluation-count/NeuralPDE evidence without claiming it reliably repairs NeuralLyapunov's stochastic benchmark.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

One-hour CI checkpoint (2026-07-16 15:09 EDT): 46 checks have succeeded, 3 were skipped, 1 failed, and 3 downstream checks remain in progress.

The changed package's complete matrix is green: OptimizationOptimisers Core passes on LTS, current, and prerelease Julia, and its QA lane passes on current Julia. Both root AD lanes are also green. NeuralPDE NNPDE2 has completed successfully.

The only failure is the standalone Documentation link check. I inspected job 87703529098 directly: Documenter received HTTP 429 responses for https://nlopt.readthedocs.io/en/latest/ and https://nlopt.readthedocs.io/en/latest/NLopt_Algorithms/. That external documentation failure is unrelated to this code change and outside the base-package CI audit scope.

Still running at this checkpoint: DiffEqFlux All, ModelingToolkit Optimization, and NeuralPDE NNPDE1 downstream tests. I am recording these as in progress, not as passed or failed.

@ChrisRackauckas-Claude
ChrisRackauckas-Claude force-pushed the agent/fix-optimisers-extra-objective-evaluation branch from 33d9282 to 329e950 Compare July 20, 2026 10:40

Copy link
Copy Markdown
Member Author

Updated in 329e950 after the inference concern:

  • x now starts as Inf, which keeps it defined for JET without restoring the discarded objective evaluation.
  • The zero-iteration fallback uses iszero(iterations), not the placeholder value.
  • A regression now verifies that a genuine Inf objective completes one iteration with exactly one objective/gradient call and no standalone objective call.

Final-source local checks:

  • Core: 38 passed, 1 pre-existing broken.
  • QA/JET: 18 passed, 1 pre-existing broken.
  • Both root/subpackage harness invocations passed.
  • Runic and git diff --check passed.

Evaluate the objective through the optimizer loop without a discarded pre-loop call, so stochastic objectives keep the expected RNG sequence and reported evaluation counts match actual work.

Preserve positive fractional maxiters values that round to zero by evaluating the initial objective only when the loop performs no iterations, and count that fallback evaluation.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Initialize the result with Inf so JET can prove the local is defined, while using the completed iteration count—not the objective value—to decide whether the zero-iteration fallback is needed. Add coverage showing a genuine infinite objective is evaluated only once.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Construct the Inf placeholder using the real scalar element type of the initial parameters. Exercise the infinite-objective regression with Float32 inputs and verify that the solution objective retains Float32.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas-Claude
ChrisRackauckas-Claude force-pushed the agent/fix-optimisers-extra-objective-evaluation branch from 329e950 to 2d45824 Compare July 20, 2026 17:24

Copy link
Copy Markdown
Member Author

Updated in 2d45824:

  • The placeholder is now convert(eltype(real(cache.u0)), Inf), so it follows the real parameter scalar type rather than defaulting to Float64.
  • The infinite-objective regression now uses Float32 parameters, returns Inf32, and asserts a Float32 solution objective.
  • Rebased onto current master (34a8fd5c).

Exact local results on Julia 1.12.6:

  • Core: 39 passed, 1 pre-existing broken; both harnesses passed.
  • Runic and git diff --check: passed.
  • QA: 18 passed, 1 failed, 1 pre-existing broken. The only failure is the rendered-public-API check, and it reproduces identically on clean origin/master; a separate mandated baseline investigation is underway. No QA check was skipped or silenced.

Copy link
Copy Markdown
Member Author

The clean-master QA investigation is complete:

SciMLTesting #27 reduces OptimizationOptimisers' false positives from 300 names to 9 inherited constants/enum values, but does not yet pass. Both current OptimizationOptimisers and OptimizationPolyalgorithms QA CI failures resolve SciMLTesting 2.3.0 and are unrelated to this PR's product-code diff. No checks were disabled or silenced.

@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review July 22, 2026 08:12
@ChrisRackauckas
ChrisRackauckas merged commit 7b1163b into SciML:master Jul 22, 2026
49 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants