Codex/matlab parity next wave - #322
Conversation
andremun
left a comment
andremun
left a comment
There was a problem hiding this comment.
Two findings from review, both worth resolving before merge — one silent behavior-changing default with no compatibility tag, one dead validation method whose docstring promises behavior that doesn't actually happen. See inline comments. Rest of the diff (2D/3D PILOT, PredictiveStage contract, TRACE3, fixture provenance) looks solid on this pass.
Generated by Claude Code
| DEFAULT_PILOT_ANALYTICS = False | ||
| DEFAULT_PILOT_N_TRIES = 5 | ||
| # Standalone PILOT multi-start count from MATLAB's ISAdefaults.m. | ||
| DEFAULT_PILOT_N_TRIES = 10 |
There was a problem hiding this comment.
DEFAULT_PILOT_N_TRIES changes 5 → 10 here, and this is a silent default change, not an opt-in one. Any existing caller using PilotOptions.default() without an explicit n_tries (the common case) now runs twice as many BFGS multi-starts for PILOT's numerical solver. Even with the same general_options.seed, doubling the restart count changes which starting point wins as "best," so the fitted A/B projection matrix — and everything downstream (PYTHIA classifiers, TRACE footprints) — can differ from before this PR, for every default-options caller.
Per this repo's CLAUDE.md production-status convention, a default-driven output change like this needs a [Behavior-changing] tag with a stated verification step (e.g. "matches the reference MATLAB fixtures before/after" or an explicit note that it doesn't and why that's acceptable) recorded in the roadmap doc. The PR body's compatibility notes don't call this out — worth adding, or reconsidering whether this should default-change vs. become an explicit opt-in for callers that want more restarts.
Generated by Claude Code
| return predicted[0], predicted[1] | ||
|
|
||
| return in_good, in_best | ||
| def _validate_explore_trace_dimensions( |
There was a problem hiding this comment.
_validate_explore_trace_dimensions is defined and documented as raising ValueError on a projection-dimension mismatch when advancing to TRACE — and explore_stage_iter()'s own docstring promises exactly that behavior — but it's never actually called anywhere in explore() or explore_stage_iter(). Grepping instance_space.py, its only occurrence is this def.
tests/test_explore_stage_iter.py reinforces the appearance that it's wired in: test_3d_stage_iter_rescores_after_trace_membership and _stub_stages both reference/monkeypatch this method as though the production pipeline invokes it.
Right now the dimension mismatch this method is meant to catch only surfaces later, incidentally, inside _explore_trace() → TraceStage.predict() — so the documented early-fail-before-PYTHIA-runs behavior doesn't actually happen. Looks like orphaned integration work: either wire the call into explore()/explore_stage_iter() where the docstring says it happens, or remove the method and correct the docstring if the later incidental check is considered sufficient.
Generated by Claude Code
andremun
left a comment
andremun
left a comment
There was a problem hiding this comment.
Continuing the earlier review (the parallel multi-angle deep-dive hit the account's 5-hour session rate limit mid-run and couldn't complete — this is a single-pass follow-up, not the full planned coverage). One more finding, on top of the two already posted (untagged DEFAULT_PILOT_N_TRIES default change, dead _validate_explore_trace_dimensions method): PythiaStage.evaluate() drops the has_ground_truth guard the old code used specifically to avoid scoring against a fabricated label. See the inline comment for details.
Generated by Claude Code
|
|
||
| MATLAB stores each confusion matrix as ``cm(:)'`` in column-major order, | ||
| hence ``[TN, FN, FP, TP]``. Every non-empty trained-classifier slot is | ||
| scored, including a reconciled training algorithm absent from the test |
There was a problem hiding this comment.
PythiaStage.evaluate() now scores a trained algorithm even when it has no real ground truth in the test set — against a truth column that's all-False by construction, not observed.
Before this PR, instance_space.py's _explore_evaluate looped only over algorithms where has_ground_truth[i] was True, leaving accuracy/precision/recall/cvcmat as NaN for a trained algorithm absent from the test metadata — the old code's own comment named this explicitly: "report NaN metrics rather than a confusion matrix computed against a fabricated label."
This PR discards that mask entirely (instance_space.py: y_raw_test, _ = self._build_test_algo_matrix(...)), and this method's own docstring states the new behavior as deliberate: "Every non-empty trained-classifier slot is scored, including a reconciled training algorithm absent from the test metadata (whose truth column is all false)."
Concretely: if algorithm X was trained but the test set's metadata simply doesn't carry performance data for X, this now reports a full confusion matrix/accuracy/precision/recall computed against an all-False "ground truth" that was never observed — a classifier predicting "good" anywhere is scored as a false positive, and one predicting "bad" everywhere gets a misleadingly perfect-looking accuracy, both against fabricated data.
What makes me want a second look specifically: every neighboring MATLAB-parity claim in this same diff cites an exact source location (e.g. "matches MATLAB's exact formulas ... core/PYTHIA.m:379-381" two paragraphs up). This one claim — "exactly as MATLAB does" — doesn't. Is there a specific MATLAB line this reconciled-all-false scoring mirrors? If MATLAB's PYTHIAevalMode genuinely does this, a citation here would put it on the same footing as the rest of the diff's claims; if not, this looks like the NaN-guard should stay.
Generated by Claude Code
andremun
left a comment
andremun
left a comment
There was a problem hiding this comment.
Second follow-up pass, now that the rate limit reset. Two more findings, on top of the three already posted. One candidate lead I investigated and rejected, worth recording so it isn't re-raised: _serialisers.py's save_instance_space_graphs() dropping the trace.use_sim branch (footprint PNGs unconditionally using data.y_bin/data.p instead of pythia.y_hat/selection0 when use_sim=True) looked like an unflagged regression at first, but checking MATLAB's actual output/scriptpng.m directly shows it always uses Ybin/P with no useSim branch at all — so this PR's change is a correctness fix (matching real MATLAB), and the previous Python behavior was the one that didn't match MATLAB. Confirmed via direct source read, not assumed.
Generated by Claude Code
| "Hyper-parameters will be auto-generated.", | ||
| expected_shape = (nalgos, n_params) | ||
| if params_array.shape != expected_shape: | ||
| raise ValueError( |
There was a problem hiding this comment.
_check_precalcparams now raises ValueError on a shape-mismatched PythiaOptions.params instead of warning and falling back to auto-generated hyperparameters (the old code: logger.warning(...); return None).
A caller who previously passed params with a shape that no longer matched (e.g. after adding an algorithm without resizing the array) used to get a logged warning and proceed with auto-tuned hyperparameters; the same input now raises and aborts build() entirely. Arguably the better failure mode — silently falling back on what's likely a real user mistake is questionable design — but it is a genuine behavior change for existing callers relying on the old permissive path, and it isn't in this PR's "Compatibility notes" section, which does call out other specific behavior changes (TRACE3 dispatch, CSV geometry, CV folds). Worth a line there, or confirmation this was already covered by an issue I'm not seeing.
Generated by Claude Code
|
|
||
| @staticmethod | ||
| def _matlab_round(value: float) -> int: | ||
| """Round halves away from zero, matching MATLAB's `round`.""" |
There was a problem hiding this comment.
Minor, non-blocking: MATLAB's round-half-away-from-zero is now implemented twice with no shared reference — this ParamSpec._matlab_round (scalar, floor(x+0.5)/ceil(x-0.5)) and trace.py's module-level _matlab_round (array input, different technique). Both exist solely to reproduce MATLAB's round() tie-breaking rule; a future floating-point edge-case fix applied to one is easy to miss applying to the other. Might be worth one shared helper, though not blocking.
Generated by Claude Code
Summary
This PR completes the third stacked MATLAB-parity branch, building on the validation/serialization and earlier big-rock branches.
MATLAB InstanceSpace source at commit
34c01293fef99b4eabd53323c393cb184cc95a8e, executed with MATLAB R2026a Update 4, remains the behavioral authority. Reviewer and GitHub findings were treated as audit leads and corrected or rejected when they conflicted with MATLAB.Main changes
PredictiveStagecontract.StageRunnerbuild-only and preserved existing plugin and persisted-model contracts.MATLAB evidence
The installed oracle is a 423-file
reference-export/v2bundle generated from:34c01293fef99b4eabd53323c393cb184cc95a8ecf3cde0da5a3067300bd94a48d4d09ff5cf20b0cd11293556b12beb63e3320094a2340ba3f7f8b7a58677ff404f20c0ba3b7350cHistorical
legacy-unknownfixtures remain regression snapshots and are not presented as MATLAB oracles.Verification
-W errormypy --strict: passed across 83 source filesIssue disposition
Implemented and ready for closure:
Closes #262
Closes #265
Resolves #272
Closes #278
Resolves #310
Closes #313
Closes #316
Closes #320
Closes #321
Partially addresses #304.
For #304, the proven per-algorithm RNG boundary and test-budget defect are corrected. MATLAB uses expected-improvement-plus, whereas scikit-optimize provides plain EI as the closest base analogue. Equal-budget candidate/objective/fold traces across repeated seeds are still required before claiming optimizer equivalence or changing defaults, so #304 should remain open.
The stacked predecessor branches already resolved #302, #314, and #317. The proposed boundary-exclusive change in #315 was rejected because it contradicted current MATLAB behavior.
After review, maintainers should reassess parent trackers #260, #270, #273, and #305. Keep #297 open while #304 remains unresolved.
Compatibility notes
0.2.1; choose the release version deliberately before publishing.