fix: seed VBPCA initialization via random_state (#109) - #113
Merged
Conversation
VBPCA's fallback random initialization (loadings/scores when no explicit init is given) and its auto-generated xprobe mask were both effectively unseeded, with no way for callers to control or reproduce them. Root cause was subtler than a single missing seed: _initialize_parameters decided whether to pass an RNG using a truthiness check on the raw `init` option. Since _build_options()'s default is the *string* "random" (truthy), the common default-configuration path was actually taking the "no new seed" branch and falling through to init_params' own internal fallback of a hardcoded `np.random.default_rng(0)` -- silently deterministic, but not user-controllable, and inconsistent with the genuinely-unseeded branch that fired when `init=None` was passed explicitly. Both "random" and None normalize to the same "no fixture" case one level down in init_params -> _normalize_init, so they should behave identically. Fix: VBPCA.__init__ gains `random_state: int | np.random.Generator | None = None`, threaded through _build_options()/_full_update.py to unconditionally seed the fallback init RNG via np.random.default_rng(random_state), and threaded into fit()'s auto xprobe-mask generation the same way. random_state follows sklearn's convention: None (default) draws fresh entropy each call. This changes the default (unseeded) case from silently-deterministic-at- seed-0 to genuinely random each call, matching what "no explicit seed" should mean and what several existing tests' names/comments already assumed was happening. Fixed 12 existing tests that implicitly depended on the old accidental determinism -- either regression tests comparing against a hardcoded value (add random_state=0, which reproduces the exact old default-path behavior since it was already np.random.default_rng(0) under the hood), or tests comparing two independent calls that need a shared seed to be comparable at all.
Covers: default is None, forwarded into resolved options, same seed -> bit-identical fit, different seeds -> diverge, None -> genuinely non-reproducible across calls, the auto xprobe-mask path specifically, accepting an existing Generator instance (not just an int), and get_params()/set_params() round-tripping. Also fixes get_options() -- it independently rebuilds the same options dict fit() constructs (rather than calling a shared helper), and was missing the random_state wire-up added to fit() in the previous commit, so it always reported random_state=None regardless of what was configured. Caught by test_random_state_forwarded_through_estimator.
This was referenced Aug 19, 2026
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.
Summary
random_state: int | np.random.Generator | Noneconstructor kwarg toVBPCA, following the sklearn convention, controlling both parameter initialization and any auto-generated xprobe mask.init="random"option was truthy, so the old ternary in_initialize_parametersnever seeded from a user-controllable source — it silently fell through toinit_params()'s internal hardcodeddefault_rng(0)fallback. Default fits were deterministic, but not configurably so.random_state=None(the default) now draws fresh entropy on everyfit(), matching sklearn semantics. Callers relying on the old accidental seed-0 determinism should passrandom_state=<int>explicitly.get_options()(a separate options-rebuild path fromfit()) was also missing therandom_statewiring — fixed so resolved options correctly reflect the configured value.Commits
feat: add random_state parameter for reproducible initialization— core fix across_full_update.py,_pca_full.py,estimators.py, plus fixups to 12 existing tests that implicitly depended on the old accidental determinism.test: verify random_state reproducibility— 9 new tests covering default/forwarding/reproducibility/divergence/xprobe-mask/Generator-instance/get_params/set_params behavior, plus theget_options()fix found while writing them.docs: changelog and limitations entry for random_state— CHANGELOGUnreleasedentry and a new bullet indocs/limitations.mdflagging the non-reproducible-by-default behavior change.Closes #109
Test plan
just ci(format-check, lint --preview, mypy --strict, test-cov) green locally: 479 passed, 90.21% coverage (gate 89%)🤖 Generated with Claude Code