Codebase review: refactor, cleanup, dedup, and behavior-risk resolution#9
Merged
Conversation
Behavior-preserving cleanup from a full codebase review. Gated by the reproducibility suite (bit-identical same-seed runs) and pyclawd golden (indicators, tcv, and all seed-1 performance scores unchanged). Dead-code removal: - Delete unused core/archive.py (never imported). - Slim knockout.py to the one live helper (noisy); drop the dead tournament machinery (is_better/comp/pcomp/knockout/NoisyReplacement/ calc_prob_relation). - Delete vendor/ego.py and vendor/smac.py (unimportable, undeclared deps) plus usage_smac.py and its permanently-skipped test scenario. - gpsaf.py: remove the broken GPSAFAnimation (referenced attributes that never exist, dragged matplotlib into every import), the always-False restart machinery, stdout debug prints, write-only attributes, dead _infill_prob_tourn best/random branches, the no-op _initialize_infill override, and all commented-out blocks. - Remove dead code across core: Target.model / Target.benchmarks, algorithm.validation, tcv.estm_scale, surrogate hot-path *_estm_error computation (nothing reads it), commented-out model-zoo blocks in defaults.py, and a stale line in indicator.kendall_tau. Latent bug fixes (no committed baseline to preserve): - optimizer.GeneticAlgorithm.optimize ran the inner pymoo GA unseeded, making SAASBO / BayesianOptimization irreproducible under global-RNG contamination; thread the run's seed in. Fixes flaky saasbo repro test. - sampling/niching.py: set n_ieq_constr (n_constr is read-only in pinned pymoo, so the fallback raised on every call) and fix the samping= typo that discarded the sampling operator. - sampling/energy.py: drop the provably-dead early-stop branch (loop always ran n_max_iter) and unused calc_potential_energy; warn instead of print. - sampling/rejection.py: drop the dead CustomLHS._calc_score override (pymoo LHS has no such hook) and the mutable default arg. Mechanical cleanup: - tcv.py: drop the undeclared autograd dependency (numpy alias only), so a clean install no longer needs autograd. - Google-style docstrings for the touched core modules; fix stale base-class docstring (wrong DOE formula and parameter name). - surrogate.py / energy.py: raise precise RuntimeError, not bare Exception. - setup.py: python_requires >=3.10, prune 3.7-3.9 classifiers, drop the __name__ reassignment hack. - test_performance.py: fix PSAF kwargs (rho_max, not the misspelled max_rho; drop GPSAF-only kwargs pymoo silently swallowed) -- effective config unchanged.
- selection.py: rename stale model_selection references (docstrings + error message) to the actual kwarg name, selection. - Add missing module docstrings: experimental/__init__.py (was empty) and experimental/acquisition.py. - Update the algorithms package docstring to cover the EGO-style family. - Remove a leftover commented-out import in ssansga2.py. - Replace shared mutable default arguments (sampling=LHS(), output=Output()) with the None-and-construct pattern in the base algorithm and SSANSGA2, and rename shadowed loop variables in the PSAF tournament.
The base _setup coupled two unrelated things: sizing the initial DOE and building the default multi-target model pool. Every EGO-style algorithm manages its own per-objective models, so each overrode _setup only to skip the pool build -- and had to re-copy the identical n_initial_doe default line (8 algorithms + experimental/bo.py). Split the base _setup: it now always sizes the DOE, and builds the default surrogate only when the new class attribute build_default_surrogate is True (extracted into _build_default_surrogate). The EGO algorithms set the flag to False and drop the copied DOE line -- ehvi/tsemo/csea/saasbo/bo lose their _setup override entirely; parego/krvea/turbo/moead_ego keep only their real setup (ref_dirs, trust-region tolerances) behind a super() call. Behaviour is preserved: the DOE value is identical, surrogate construction consumes no run RNG, and the EGO algorithms never used the default pool they now skip. Verified by all 11 reproducibility tests (bit-identical same-seed) and pyclawd golden (seed-1 performance scores unchanged). Side benefit: the reproducibility suite is ~4x faster now that those algorithms no longer build a ~40-model pool they discard.
The eight EGO-style algorithms were written in one session and shared several
near-verbatim blocks. Extract them into a private _ego.py module:
- default_kriging(): the Kriging(Exponential()) prototype (6 copies)
- fit_per_objective(): deepcopy-per-objective fit loop (4 copies)
- predict_mu_sigma(): mean+std prediction -- now ONE predict(var=True) per
model instead of two (the mean from var=True is
bit-identical to a plain predict, verified), halving the
Kriging predict cost in ehvi/moead_ego/tsemo (3 copies)
- lhs_local_pool(): LHS + elite-perturbation candidate pool, drawing the RNG
strictly LHS -> integers -> standard_normal (3 copies)
- front_and_hv(): non-dominated front + HV reference point (2 copies)
- pareto_optimum() / best_optimum(): the _set_optimum bodies (7 copies)
Every extraction is a literal move preserving the exact RNG draw order, so
same-seed runs stay bit-identical: verified by all 11 reproducibility tests,
the fidelity suite, and pyclawd golden (seed-1 performance scores unchanged).
Each finding was resolved by measuring the effect, not just applying the "obvious" fix -- two of them turned out to be intentional and were preserved. PSAF (kept the effective behavior, fixed the naming/dead code): - The `bias = max(rho_max, r2)` "cap vs floor" was a floor by design: PSAF exploits the surrogate at a base rate and only raises it when the surrogate clearly beats the KNN baseline. Reinterpreting it as a cap (min) collapsed PSAF(GA) from ~4x better than GA to no better. Kept the operator, renamed the parameter rho_max -> rho_min to match the floor semantics, documented why. - The two Kriging pool members used a string regr that pydacefit silently failed to fit, so PSAF has always run an RBF+KNN pool. Re-enabling Kriging with proper regression objects *regressed* PSAF on the benchmarks, so the dead entries are removed rather than "fixed" -- the pool is honestly RBF+KNN. - Thread the run seed into the inner algorithm so a constructor-set seed (not only minimize(seed=)) is honoured, matching GPSAF. (minimize path unchanged.) Genuine fixes (golden updated -- intended, assertions still pass by wide margin): - KRVEA diversity branch topped up from the uncertainty ranking so the batch always spends its full budget (paper-faithful): seed-1 IGD 0.132 -> 0.144. - SSANSGA2 KMeans seed derived from the run RNG instead of a fixed 0, matching the file's own inner-NSGA2 seeding philosophy: seed-1 IGD 0.0079 -> 0.0094. Constraint modelling and sampling reproducibility: - defaults.py: apply Plog as norm_y (a target transform for constraint violations) instead of norm_X, and keep the caller's design-space norm_X. - rejection/energy sampling: thread random_state through the LHS draw and the max-distance first pick, replacing a global np.random.randint. Tooling: - Separate golden into its own tier (excluded from fast/default) per AGENTS.md, and split CI's test step into a unit run and a dedicated golden run.
blankjul
changed the title
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.
Codebase review + refactor/cleanup
A full review of the repo (8 parallel reviewers across core / algorithms / experimental /
sampling / tests / quality / dead-code) turned up ~60 findings. This PR lands all of them
across five waves. Net ~−450 lines.
Every wave was gated by the strongest oracle for what it touched:
pyclawd golden— the seed-1 performance scores of every algorithm pair,pyclawd check— format / lint / typecheck / descriptions / tests.Wave 1 — remove dead code, fix latent bugs, mechanical cleanup
core/archive.py,vendor/ego.py,vendor/smac.py+usage_smac.py.knockout.pyto the one live helper; delete the brokenGPSAFAnimation(draggedmatplotlibinto every import), the always-False restart machinery, debug prints, dead branches.core(Target.model/benchmarks,algorithm.validation,tcv.estm_scale, the unread*_estm_errorhot path, commented model-zoo blocks).raised on every call + its
samping=typo, dead early-stop in the energy sampler.autograddependency; fix stalesetup.pymetadata (python_requires ≥3.10).Wave 2 — docs & naming consistency
model_selection→selection, missing module docstrings, shared mutable default args, shadowed vars.Wave 3a —
build_default_surrogatehookSplit the base
_setupso DOE sizing is no longer welded to the default-pool build; 9 duplicatedDOE blocks → 0. Side benefit: the reproducibility suite is ~4× faster.
Wave 3b — shared EGO machinery (
algorithms/_ego.py)Extract six near-verbatim blocks (
default_kriging,fit_per_objective,predict_mu_sigma,lhs_local_pool,front_and_hv,pareto_optimum/best_optimum).predict_mu_sigmaalso halvesthe Kriging predict cost in 3 algorithms (mean is bit-identical, verified). Exact RNG order preserved.
Wave 4 — resolve the behavior-risk findings (measured, not assumed)
Two "obvious fixes" turned out to be intentional behaviour and were preserved — measuring first
is what caught it:
max(rho_max, r2)is a floor, not a cap, by design: the surrogate is exploited at abase rate and only raised when it clearly beats the KNN baseline. The "cap" reinterpretation
collapsed PSAF(GA) from ~4× better to no better. Kept the operator; renamed
rho_max→rho_min.regrentries silently never fit, so PSAF has always runRBF+KNN; re-enabling Kriging regressed it. Removed the dead entries (honest RBF+KNN pool).
KRVEA now tops up its diversity batch to spend the full budget (paper-faithful; seed-1 IGD
0.132→0.144); SSANSGA2 derives its KMeans seed from the run RNG like its inner NSGA2
(0.0079→0.0094). Both remain ~60–90× better than NSGA2.
defaults.pyappliesPlogasnorm_y(a constraint-violation target transform) and keepsthe caller's design-space
norm_X.random_statethreaded through the rejection/energy samplers.Notes
clean fix remains releasing/pinning pysurrogate/pydacefit/pysampling/ezmodel.
findings.md,runs/,docs/*.png,.claude/skills/) intentionally leftuncommitted.
🤖 Generated with Claude Code