refactor(optimizers): delete the cached typed options copy, 42 updateoptions overrides to 6 - #2058
Closed
ooples wants to merge 6 commits into
Closed
refactor(optimizers): delete the cached typed options copy, 42 updateoptions overrides to 6#2058ooples wants to merge 6 commits into
ooples wants to merge 6 commits into
Conversation
Seed is on the shared options and OptimizerBase.CreateSearchRandom documents the
contract: "a run is a random variable. One that cannot be repeated cannot be debugged."
The function-minimisation overloads keep it. Optimize(inputData) did not, because three
generators on that path were built without ever consulting the seed.
OptimizerBase ran
_model = model;
Random = new();
NumOps = ...;
Options = options ?? new ...;
so the shared generator was constructed on the line ABOVE the Options assignment. It
could not have read a seed even had it tried, and RandomlySelectFeatures and
InitializeRandomSolution both draw from it. Reordered so Options is assigned first and
the generator derives from Options.Seed, via RandomHelper as the coding standard
requires rather than a bare new Random().
The candidate-perturbation loop in BuildDataDerivedRandomParameters created its own
CreateSecureRandom(). It now draws from the shared generator, since that perturbation
decides the parameters every candidate starts from.
ParticleSwarmOptimizer built its own field with CreateSecureRandom() in both
constructors, ignoring the seed outright; velocity initialisation and the cognitive and
social draws all come from it. Both constructors now derive it from the options, and
UpdateOptions refreshes it, because a generator still running on the previous seed would
contradict the options the optimizer reports. The field loses readonly for that reason.
DifferentialEvolutionOptimizer needed nothing - it already takes CreateSearchRandom.
Two tests, both failing before this change and passing after. The narrow one asserts the
generators directly and is the honest statement of what changed; without the fix it
reports
Expected: Tuple (2072998746, 0.30262624952133105)
Actual: Tuple (493046785, 0.83774535955756224)
from two constructions with the SAME seed. The broader one asserts a same-seed
Optimize(inputData) returns the same BestFitnessScore, which is the behaviour callers
actually want. Equality rather than a tolerance is deliberate: a seed either determines
the run or it does not.
Verified: 1690 of 1690 optimizer tests pass, so seeding the shared generator regresses
nothing across the other optimizers that inherit it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk
…r optimizer Addresses both review findings, and takes the fix to the level it belonged at. Deriving one class's generator from the seed was the wrong shape. A survey of all 60 files under src/Optimizers found the same private-generator pattern in SimulatedAnnealingOptimizer, which would have kept the defect after a particle-swarm-only fix, and nothing stops the next optimizer from adding another. So both private generators are deleted and both classes now draw from the seeded Random that OptimizerBase already owns. There is no longer a per-optimizer generator to forget to seed. OptimizationDataBatcher was the only other hit and already seeds correctly. That also dissolves the first review finding rather than patching it. UpdateOptions no longer rebuilds anything, because the class holds nothing to rebuild. The second half of that finding was real and is fixed at the same single point: Deserialize handed restored options to UpdateOptions while the base kept its constructor values, so a restored optimizer reported one seed and drew from another. The base now adopts the restored options before handing them on, which fixes it for every optimizer at once. Deliberately narrow - the collaborators cached at construction (FitDetector, FitnessCalculator, the statistics options) are left alone, since replacing live collaborators mid-lifetime is a separate concern from reproducibility. The second finding was correct and the remarks were stale: they claimed Optimize(inputData) was not reproducible and that the test only inspected generators. Both were written while a sync test carrying Fact(Timeout) was failing with "Tests marked with Timeout are only supported for async tests" - an infrastructure error I read as an assertion failure. The tests are async with Task.Yield now, the Timeout guard works, and the remarks describe what is actually asserted. Tests follow the same move: the generator test covers particle swarm AND simulated annealing, because the contract is the base class's rather than either optimizer's. Verified: 1691 of 1691 optimizer tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk
…ze clobbering collaborators The review was right that adopting Options and Random while leaving PredictionOptions, ModelStatsOptions, FitDetector, FitnessCalculator and ModelCache on their constructor values is a torn state. Adopting them literally, however, is worse - and the difference is measurable. Interface-typed properties carry no type information through the options JSON, so Newtonsoft rebuilds each as its property initializer's default. Measured: an optimizer configured with MeanSquaredErrorFitnessCalculator serializes, and the payload comes back holding RSquaredFitnessCalculator, the default. Adopting the payload's collaborators would therefore swap a caller's evaluator, fit detector and cache for defaults on every deserialize - silently, and on a path whose entire purpose is to restore configuration. An earlier probe of mine looked like they round-tripped; they did not. Both sides simply held the default, so "non-null" read as "restored". So the restored payload contributes its VALUES, and the live collaborators are carried onto it before adoption. Options and the cached fields then agree, which is the property the review actually asked for, without a deserialize quietly replacing configuration. Adoption itself is now one method. ApplyOptions defines how an option set becomes state and is the only thing that does; the constructor and Deserialize both route through it, so they cannot drift and a future option-derived field is restored for free by being added there. Declared with MemberNotNull rather than a null-forgiving operator, which the coding standard forbids and which several call sites in this repo already model. Verified: 1692 of 1692 optimizer tests pass; src builds clean on net10.0, net8.0 and net471, so MemberNotNull holds on the framework target too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk
…rror
A fix that has to be remembered is a fix that comes back. Two optimizers had already
built private generators that no seed could reach, and nothing stopped a third. AIDN077
closes the class rather than the instances.
Inside any type deriving from OptimizerBase, both spellings are refused:
new Random(...)
RandomHelper.CreateSecureRandom() / CreateSeededRandom(...)
because the base already exposes a Random derived from Options.Seed and re-derives it
when options are restored. OptimizerBase itself is exempt - it is the one type that must
build the generator, and where the seed is honoured for everything below it.
This is a HARD gate, unlike its neighbours. TreatWarningsAsErrors is on and AIDN070
through AIDN076 are each listed in WarningsNotAsErrors, because existing code violates
them and they are being paid down gradually. AIDN077 has zero violations the moment it
lands, so it is deliberately NOT added to that list and fails the build from day one.
Grandfathering it would have meant shipping a rule that never bites.
Proven by reintroducing the exact defect it exists to stop:
error AIDN077: 'RandomHelper.CreateSecureRandom()' creates a generator of its own;
draw from the inherited OptimizerBase.Random, which is built from Options.Seed
error AIDN077: 'new Random(...)' creates a generator of its own; ...
Errors, not warnings, and the build exits nonzero. With the defect removed the same
build is clean on net10.0, net8.0 and net471, which also shows OptimizerBase's own
exemption works rather than merely being written down.
Registered in AnalyzerReleases.Unshipped.md. The category there must match the
descriptor's Category constant exactly - AiDotNet.GoldenPattern, singular - or the
release tracker reports RS2001 against a rule that is otherwise fine.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk
…not the initial one Chasing whether the 42 optimizers overriding UpdateOptions leave option-derived state stale, this looked like a defect: the constructor seeds _currentInertia and its siblings from Initial* on the options, and no UpdateOptions override re-runs InitializeAdaptiveParameters, so a restored optimizer reads like it keeps the configuration it was BUILT with. It does not. Measured across a deserialize, driving the source's live value away from its initial one first: BEFORE cur=0.91 opt=0.91 | AFTER cur=0.137 opt=0.42 | SRC cur=0.137 opt=0.42 The state layer carries the live values, so a mid-run optimizer serialized at inertia 0.137 comes back at 0.137 rather than at either side's Initial*. There is no staleness to fix. Which makes the obvious "fix" harmful. Calling ResetAdaptiveParameters() from Deserialize overwrites restored state, silently rewinding a resumed optimizer to the start of its schedule - and I had written exactly that before measuring. This test refuses it: against that implementation it fails with Expected: 0.13700000000000001 Actual: 0.41999999999999998 and passes against the current one. The distinction only appears when the live value is driven away from the initial value, which is why the test sets it explicitly instead of trusting a short run to diverge - an earlier attempt that skipped that step passed under both implementations and proved nothing. No production change. 379 optimizer tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk
…options overrides to 6
UpdateOptions was abstract, so all 42 concrete optimizers implemented it, and 36 of those
were the same eight lines: downcast the options, assign them to a private typed field,
throw otherwise. That field was a second copy of state the base already owns, and keeping
two copies in step is the only reason the method existed. Every new optimizer inherited
the obligation, and any change to how options are held meant 42 edits.
Those 36 now read their typed options through a computed property over the base's single
instance:
private AdamOptimizerOptions<T, TInput, TOutput> _options
=> (AdamOptimizerOptions<T, TInput, TOutput>)Options;
so the override has nothing to do and is deleted. UpdateOptions becomes virtual with a
no-op default, to be overridden only to REACT to a new option set. Six do: ADMM and
ProximalGradientDescent rebuild a regularizer, Bayesian updates a kernel,
GradientBasedOptimizerBase caches a scheduler mode, Lion re-seeds adaptive parameters,
Rprop validates hyperparameters. Those keep their field and their override.
This closes a latent bug rather than only removing noise. The constructors read
: base(model, options ?? new())
{
_options = options ?? new AdamOptimizerOptions<T, TInput, TOutput>();
which builds TWO different default instances whenever the caller passes null - the base
holding one, the derived class the other, and a change to either invisible to the other.
There is now one instance and no way to desynchronise them.
Adam8BitOptimizer's hand-rolled Deserialize also kept its own copy of the options JSON.
The read stays, because the stream position depends on it and a malformed payload must
still fail there, but the value is discarded: base.Deserialize has already adopted the
authoritative copy.
Stacked on fix/pso-honours-its-seed and depends on it. That branch makes the base adopt
restored options in Deserialize; without it, a computed property would read the
constructor's options after a restore rather than the restored ones.
Verified: 1693 of 1693 optimizer tests pass, and src builds clean on net10.0, net8.0 and
net471. Net -783 lines.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (41)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Folded into #2055 rather than run a second multi-hour CI cycle on work that cannot build without it. The commit Splitting them was my mistake: the refactor depends on #2055's |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
UpdateOptionswasabstract, so all 42 concrete optimizers implemented it — and 36 were the same eight lines:That private field is a second copy of state the base already owns, and keeping two copies in step is the only reason the method existed. Every new optimizer inherited the obligation, and any change to how options are held meant 42 edits.
The change
The 36 read their typed options through a computed property over the base's single instance:
The override then has nothing to do and is deleted.
UpdateOptionsbecomesvirtualwith a no-op default, to be overridden only to react to a new option set.Six still do, and keep both field and override: ADMM and ProximalGradientDescent rebuild a regularizer, Bayesian updates a kernel, GradientBasedOptimizerBase caches a scheduler mode, Lion re-seeds adaptive parameters, Rprop validates hyperparameters.
It closes a latent bug, not just noise
The constructors read:
When the caller passes
nullthose are two different default instances — the base holding one, the derived class the other, each blind to changes in the other. There is now one instance and no way to desynchronise them.Adam8BitOptimizer's hand-rolledDeserializekept its own copy of the options JSON too. The read stays — the stream position depends on it and a malformed payload must still fail there — but the value is discarded, sincebase.Deserializehas already adopted the authoritative copy.Verification
srcbuilds clean on net10.0, net8.0 and net471.Note on how this was produced
The mechanical conversion was scripted, but two passes had to be redone: a first regex over-reached into the six exception files and stripped constructor assignments they still need, and
RootMeanSquarePropagationOptimizeris apartial classwhose override lives in another file, so a per-file pass missed it. Both were caught by the compiler, not by inspection — worth knowing if this pattern is applied elsewhere.🤖 Generated with Claude Code
https://claude.ai/code/session_01U1HFtoj81avXvuLebTpWyk