From 73f788f1807084f10fd766b6bb657e90c9d7e16f Mon Sep 17 00:00:00 2001 From: vahid-ahmadi Date: Wed, 16 Sep 2026 13:02:37 +0100 Subject: [PATCH 1/3] Give each per-variable QRF model its own seed Every per-variable model builds its own generator from the seed it is given, and all of them were handed self.seed. They therefore drew the same random quantiles in the same row order, so variables imputed together came out rank-comonotonic whatever their dependence in the donor: three targets with nil conditional dependence reproduced at 0.71 Spearman, 0.11 after this change. The offset follows the convention already used for the subsampling seed in _apply_max_train_samples. QRF also now accepts a seed argument; there was previously no way for a caller to vary the draws. Fixes #207 --- changelog.d/207.fixed.md | 1 + microimpute/models/qrf.py | 32 +++++++++++++++++++---- tests/test_models/test_qrf.py | 48 +++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 changelog.d/207.fixed.md diff --git a/changelog.d/207.fixed.md b/changelog.d/207.fixed.md new file mode 100644 index 00000000..58db8524 --- /dev/null +++ b/changelog.d/207.fixed.md @@ -0,0 +1 @@ +Per-variable QRF models now derive distinct seeds, so variables imputed together no longer share one random quantile per row and come out comonotonic. `QRF` also accepts a `seed` argument. diff --git a/microimpute/models/qrf.py b/microimpute/models/qrf.py index 8edc5d08..da9f169d 100644 --- a/microimpute/models/qrf.py +++ b/microimpute/models/qrf.py @@ -10,7 +10,7 @@ from quantile_forest import RandomForestQuantileRegressor from sklearn.ensemble import RandomForestClassifier -from microimpute.config import VALIDATE_CONFIG +from microimpute.config import RANDOM_STATE, VALIDATE_CONFIG from microimpute.models.imputer import Imputer, ImputerResults try: @@ -607,6 +607,7 @@ def __init__( batch_size: Optional[int] = None, cleanup_interval: int = 10, max_train_samples: Optional[int] = None, + seed: Optional[int] = RANDOM_STATE, ) -> None: """Initialize the QRF model. @@ -618,8 +619,11 @@ def __init__( max_train_samples: If set, subsample X_train to at most this many rows before fitting. Reduces memory and training time while preserving sequential covariance structure. + seed: Base random seed. Each imputed variable is given a distinct + seed derived from it, so variables imputed together draw + independently. Pass None for non-reproducible draws. """ - super().__init__(log_level=log_level) + super().__init__(log_level=log_level, seed=seed) self.models = {} self.log_level = log_level self.memory_efficient = memory_efficient @@ -695,20 +699,38 @@ def _encode_imputed_variable( return data + def _seed_for_variable(self, variable: str) -> Optional[int]: + """Derive a distinct seed for one imputed variable. + + Each per-variable model builds its own generator from the seed it is + given. Handing every variable the same seed makes them draw the same + random quantiles in the same row order, so variables imputed together + come out comonotonic regardless of their dependence in the donor. The + offset follows the same convention as the subsampling seed below. + """ + if self.seed is None: + return None + try: + variable_offset = (self.imputed_variables or []).index(variable) + except ValueError: + variable_offset = 0 + return self.seed + variable_offset + def _create_model_for_variable(self, variable: str, **kwargs) -> Any: """Create the appropriate model (classifier or regressor) based on variable type.""" categorical_targets = getattr(self, "categorical_targets", {}) boolean_targets = getattr(self, "boolean_targets", {}) + seed = self._seed_for_variable(variable) if variable in categorical_targets: # Use classifier for categorical targets - return _RandomForestClassifierModel(seed=self.seed, logger=self.logger) + return _RandomForestClassifierModel(seed=seed, logger=self.logger) elif variable in boolean_targets: # Use classifier for boolean targets - return _RandomForestClassifierModel(seed=self.seed, logger=self.logger) + return _RandomForestClassifierModel(seed=seed, logger=self.logger) else: # Use QRF for numeric targets - return _QRFModel(seed=self.seed, logger=self.logger) + return _QRFModel(seed=seed, logger=self.logger) def _fit_model( self, diff --git a/tests/test_models/test_qrf.py b/tests/test_models/test_qrf.py index e3099f75..3e2f0b8a 100644 --- a/tests/test_models/test_qrf.py +++ b/tests/test_models/test_qrf.py @@ -1649,3 +1649,51 @@ def test_qrf_fit_predict_with_max_train_samples() -> None: assert result.shape == (n_test, 1) assert not result.isna().any().any() + + +def test_per_variable_models_draw_independently() -> None: + """Variables imputed together must not share one random quantile per row. + + Every per-variable model builds its own generator from the seed it is + given. When they all receive the same seed they draw the same quantiles in + the same row order, so the imputed variables come out comonotonic whatever + their dependence in the donor. + """ + rng = np.random.default_rng(7) + n = 1500 + predictors = pd.DataFrame( + {"inc": rng.normal(30, 8, n), "age": rng.normal(45, 12, n)} + ) + targets = ["savings", "property_wealth", "corporate_wealth"] + train = predictors.copy() + for variable in targets: + # Shared signal, independent shocks: the conditional dependence is nil. + train[variable] = 0.5 * predictors["inc"] + rng.normal(0, 10, n) + + test = pd.DataFrame({"inc": rng.normal(30, 8, 600), "age": rng.normal(45, 12, 600)}) + + model = QRF(log_level="WARNING") + imputations = model.fit(train, ["inc", "age"], targets).predict(test) + + ranks = imputations[targets].rank().corr() + off_diagonal = [ + abs(ranks.loc[a, b]) for i, a in enumerate(targets) for b in targets[i + 1 :] + ] + assert max(off_diagonal) < 0.4, ( + "imputed variables are comonotonic; per-variable models are sharing " + f"a seed (rank correlations {off_diagonal})" + ) + + +def test_seed_is_configurable_and_reproducible() -> None: + """QRF should accept a seed, and the same seed should reproduce draws.""" + rng = np.random.default_rng(3) + n = 400 + train = pd.DataFrame({"x": rng.normal(size=n)}) + train["y"] = train["x"] + rng.normal(0, 1, n) + test = pd.DataFrame({"x": rng.normal(size=120)}) + + first = QRF(log_level="WARNING", seed=1234).fit(train, ["x"], ["y"]).predict(test) + second = QRF(log_level="WARNING", seed=1234).fit(train, ["x"], ["y"]).predict(test) + + np.testing.assert_allclose(first["y"], second["y"]) From 146f0c41d4bf0a016d63ba84c24460d89b0d4604 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Juaristi?= <127882282+juaristi22@users.noreply.github.com> Date: Thu, 17 Sep 2026 11:35:06 +0100 Subject: [PATCH 2/3] Fix issues from review: bound QRF seeds and preserve tuning streams --- changelog.d/207.fixed.md | 1 + .../cross-validation.md | 12 +- docs/imputation-benchmarking/preprocessing.md | 8 +- .../imputation-benchmarking/visualizations.md | 8 +- docs/models/imputer/implement-new-model.md | 4 +- docs/use_cases/index.md | 12 +- microimpute/models/qrf.py | 20 ++-- tests/test_models/test_qrf.py | 108 ++++++++++++++++++ 8 files changed, 138 insertions(+), 35 deletions(-) diff --git a/changelog.d/207.fixed.md b/changelog.d/207.fixed.md index 58db8524..7dd1efda 100644 --- a/changelog.d/207.fixed.md +++ b/changelog.d/207.fixed.md @@ -1 +1,2 @@ Per-variable QRF models now derive distinct seeds, so variables imputed together no longer share one random quantile per row and come out comonotonic. `QRF` also accepts a `seed` argument. +Derived seeds stay within the supported uint32 range and are used consistently during numeric and classification tuning and target-specific subsampling. diff --git a/docs/imputation-benchmarking/cross-validation.md b/docs/imputation-benchmarking/cross-validation.md index 8da745ff..c1b6f93e 100644 --- a/docs/imputation-benchmarking/cross-validation.md +++ b/docs/imputation-benchmarking/cross-validation.md @@ -41,23 +41,23 @@ Returns a dictionary containing separate results for each metric type: ```python { "quantile_loss": { - "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (mean across folds) + "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (mean across folds) "results_std": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (std across folds) "mean_train": float, "mean_test": float, "std_train": float, "std_test": float, - "variables": List[str] # numerical variables evaluated + "variables": List[str], # numerical variables evaluated }, "log_loss": { - "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles + "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles "results_std": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (std across folds) "mean_train": float, "mean_test": float, "std_train": float, "std_test": float, - "variables": List[str] # categorical variables evaluated - } + "variables": List[str], # categorical variables evaluated + }, } ``` @@ -77,7 +77,7 @@ results = cross_validate_model( data=diabetes_df, predictors=["age", "sex", "bmi", "bp"], imputed_variables=["s1", "s4"], - n_splits=5 + n_splits=5, ) # Check performance for numerical variables diff --git a/docs/imputation-benchmarking/preprocessing.md b/docs/imputation-benchmarking/preprocessing.md index 736d73ed..05693943 100644 --- a/docs/imputation-benchmarking/preprocessing.md +++ b/docs/imputation-benchmarking/preprocessing.md @@ -117,10 +117,10 @@ result = autoimpute( predictors=["age", "education"], imputed_variables=["income", "wealth"], preprocessing={ - "income": "log", # Log transform (positive values only) - "wealth": "asinh", # Asinh transform (handles zeros/negatives) - "age": "normalize" # Z-score normalization - } + "income": "log", # Log transform (positive values only) + "wealth": "asinh", # Asinh transform (handles zeros/negatives) + "age": "normalize", # Z-score normalization + }, ) ``` diff --git a/docs/imputation-benchmarking/visualizations.md b/docs/imputation-benchmarking/visualizations.md index 21dde021..fe8f87b8 100644 --- a/docs/imputation-benchmarking/visualizations.md +++ b/docs/imputation-benchmarking/visualizations.md @@ -83,11 +83,7 @@ comparison_viz = method_comparison_results( ) # Generate plot -fig = comparison_viz.plot( - title="Method comparison", - show_mean=True, - plot_type="bar" -) +fig = comparison_viz.plot(title="Method comparison", show_mean=True, plot_type="bar") fig.show() # Get summary statistics @@ -165,7 +161,7 @@ perf_viz = model_performance_results( results=cv_results, model_name="QRF", method_name="Cross-validation", - metric="quantile_loss" + metric="quantile_loss", ) fig = perf_viz.plot(title="QRF performance") diff --git a/docs/models/imputer/implement-new-model.md b/docs/models/imputer/implement-new-model.md index edf21254..794d6193 100644 --- a/docs/models/imputer/implement-new-model.md +++ b/docs/models/imputer/implement-new-model.md @@ -73,9 +73,7 @@ class NewModelResults(ImputerResults): except Exception as e: self.logger.error(f"Error during Model prediction: {str(e)}") - raise RuntimeError( - f"Failed to predict with Model: {str(e)}" - ) from e + raise RuntimeError(f"Failed to predict with Model: {str(e)}") from e ``` ## Implementing the main model class diff --git a/docs/use_cases/index.md b/docs/use_cases/index.md index 32c7af96..2c9ff07c 100644 --- a/docs/use_cases/index.md +++ b/docs/use_cases/index.md @@ -23,7 +23,7 @@ Before imputation, make sure both datasets have compatible variables. Identify c ```python # Identify common variables -common_variables = ['age', 'income', 'education', 'marital_status', 'region'] +common_variables = ["age", "income", "education", "marital_status", "region"] # Ensure variable formats match (example: education coding) education_mapping = { @@ -31,19 +31,19 @@ education_mapping = { 2: "high_school", 3: "some_college", 4: "bachelor", - 5: "graduate" + 5: "graduate", } # Apply standardization to both datasets for dataset in [scf_data, cps_data]: - dataset['education'] = dataset['education'].map(education_mapping) + dataset["education"] = dataset["education"].map(education_mapping) # Convert income to same units (thousands) - if 'income' in dataset.columns: - dataset['income'] = dataset['income'] / 1000 + if "income" in dataset.columns: + dataset["income"] = dataset["income"] / 1000 # Identify target variable in donor dataset -target_variable = ['networth'] +target_variable = ["networth"] ``` ## Performing imputation diff --git a/microimpute/models/qrf.py b/microimpute/models/qrf.py index da9f169d..76bc4768 100644 --- a/microimpute/models/qrf.py +++ b/microimpute/models/qrf.py @@ -706,15 +706,18 @@ def _seed_for_variable(self, variable: str) -> Optional[int]: given. Handing every variable the same seed makes them draw the same random quantiles in the same row order, so variables imputed together come out comonotonic regardless of their dependence in the donor. The - offset follows the same convention as the subsampling seed below. + offset is shared with target-specific subsampling and wraps within + sklearn's uint32 seed range. """ if self.seed is None: return None + if not isinstance(self.seed, (int, np.integer)) or not 0 <= self.seed < 2**32: + raise ValueError("seed must be an integer from 0 to 2**32 - 1 or None") try: variable_offset = (self.imputed_variables or []).index(variable) except ValueError: variable_offset = 0 - return self.seed + variable_offset + return (int(self.seed) + variable_offset) % 2**32 def _create_model_for_variable(self, variable: str, **kwargs) -> Any: """Create the appropriate model (classifier or regressor) based on variable type.""" @@ -822,12 +825,7 @@ def _target_fit_data( self.max_train_samples is not None and len(target_train) > self.max_train_samples ): - try: - variable_offset = (self.imputed_variables or []).index(variable) - except ValueError: - variable_offset = 0 - seed = None if self.seed is None else self.seed + variable_offset - rng = np.random.default_rng(seed) + rng = np.random.default_rng(self._seed_for_variable(variable)) sel = rng.choice( len(target_train), size=self.max_train_samples, replace=False ) @@ -1487,7 +1485,9 @@ def objective(trial: optuna.Trial) -> float: y_val = X_val_fold[var] # Create and fit QRF model with trial parameters - model = _QRFModel(seed=self.seed, logger=self.logger) + model = _QRFModel( + seed=self._seed_for_variable(var), logger=self.logger + ) model.fit( X_train_augmented[encoded_predictors], X_train_fold[var], @@ -1645,7 +1645,7 @@ def objective(trial: optuna.Trial) -> float: # Create and fit RFC model with trial parameters model = _RandomForestClassifierModel( - seed=self.seed, logger=self.logger + seed=self._seed_for_variable(var), logger=self.logger ) # Determine variable type and fit appropriately diff --git a/tests/test_models/test_qrf.py b/tests/test_models/test_qrf.py index 3e2f0b8a..cd203360 100644 --- a/tests/test_models/test_qrf.py +++ b/tests/test_models/test_qrf.py @@ -1697,3 +1697,111 @@ def test_seed_is_configurable_and_reproducible() -> None: second = QRF(log_level="WARNING", seed=1234).fit(train, ["x"], ["y"]).predict(test) np.testing.assert_allclose(first["y"], second["y"]) + + +@pytest.mark.parametrize("target_type", ["numeric", "boolean", "categorical"]) +def test_qrf_max_seed_supports_multiple_targets(target_type: str) -> None: + """Every valid sklearn seed must support reproducible multi-target fits.""" + rng = np.random.default_rng(71) + data = pd.DataFrame( + {name: rng.normal(size=120) for name in ["x", "first", "second"]} + ) + if target_type == "boolean": + data[["first", "second"]] = data[["first", "second"]] > 0 + elif target_type == "categorical": + for name in ["first", "second"]: + data[name] = np.where(data[name] > 0, "yes", "no") + + predictions = [] + for _ in range(2): + fitted = QRF(seed=2**32 - 1).fit( + data, ["x"], ["first", "second"], n_estimators=12 + ) + seeds = [model.seed for model in fitted.models.values()] + assert len(set(seeds)) == 2 + assert all(0 <= seed < 2**32 for seed in seeds) + predictions.append(fitted.predict(data[["x"]].iloc[:20])) + pd.testing.assert_frame_equal(*predictions) + assert not predictions[0].isna().any().any() + + +@pytest.mark.parametrize("seed", [None, 0, 42]) +def test_qrf_child_seeds_preserve_existing_seed_values(seed) -> None: + """Ordinary seeds and entropy-based draws retain their established meaning.""" + model = QRF(seed=seed) + model.imputed_variables = ["first", "second"] + expected = [None, None] if seed is None else [seed, seed + 1] + assert [ + model._seed_for_variable(name) for name in model.imputed_variables + ] == expected + + +@pytest.mark.parametrize("seed", [-1, 2**32, 1.5]) +def test_qrf_invalid_base_seed_is_not_normalized(seed) -> None: + """Wrapping child seeds must not silently accept invalid sklearn base seeds.""" + rng = np.random.default_rng(3) + data = pd.DataFrame({"x": rng.normal(size=30), "y": rng.normal(size=30)}) + with pytest.raises(RuntimeError): + QRF(seed=seed).fit(data, ["x"], ["y"], n_estimators=5) + + +@pytest.mark.parametrize("target_type", ["numeric", "boolean"]) +def test_qrf_tuning_uses_distinct_target_seeds(monkeypatch, target_type: str) -> None: + """Real Optuna folds must fit the same independent streams as final models.""" + from microimpute.models.qrf import _RandomForestClassifierModel + + rng = np.random.default_rng(73) + data = pd.DataFrame( + {name: rng.normal(size=100) for name in ["x", "first", "second"]} + ) + model = QRF(seed=123) + model.imputed_variables = ["first", "second"] + if target_type == "numeric": + internal_model = _QRFModel + tune = model._tune_qrf_hyperparameters + else: + data[["first", "second"]] = data[["first", "second"]] > 0 + model.boolean_targets = {"first": {}, "second": {}} + internal_model = _RandomForestClassifierModel + tune = model._tune_rfc_hyperparameters + + fitted_seeds = [] + original_fit = internal_model.fit + + def record_fit(self, X, y, **kwargs): + fitted_seeds.append((y.name, self.seed)) + return original_fit(self, X, y, **kwargs) + + monkeypatch.setattr(internal_model, "fit", record_fit) + tune(data, ["x"], ["first", "second"], n_cv_folds=2, n_trials=1) + assert fitted_seeds == [("first", 123), ("second", 124)] * 2 + + +def test_qrf_target_subsampling_uses_bounded_child_seed(monkeypatch) -> None: + """Filtered training rows and their model use one consistent child seed.""" + rng = np.random.default_rng(9) + data = pd.DataFrame( + {name: rng.normal(size=120) for name in ["x", "first", "second"]} + ) + fitted_indices = {} + original_fit = _QRFModel.fit + + def record_fit(self, X, y, **kwargs): + fitted_indices[y.name] = X.index.to_numpy() + return original_fit(self, X, y, **kwargs) + + monkeypatch.setattr(_QRFModel, "fit", record_fit) + QRF(seed=2**32 - 1, max_train_samples=50).fit( + data, + ["x"], + ["first", "second"], + target_filters={ + name: np.ones(len(data), dtype=bool) for name in ["first", "second"] + }, + n_estimators=12, + ) + # The second stream wraps to zero at the uint32 boundary. + expected_indices = np.random.default_rng(0).choice( + len(data), size=50, replace=False + ) + np.testing.assert_array_equal(fitted_indices["second"], expected_indices) From a8c94bdfd80de4d988c034258c3a3484eac43d8c Mon Sep 17 00:00:00 2001 From: vahid-ahmadi Date: Mon, 21 Sep 2026 10:34:05 +0100 Subject: [PATCH 3/3] Fail loudly on an unknown variable and an invalid seed Per review, two ways the fix could be undone silently. _seed_for_variable fell back to offset 0 when a variable was not in imputed_variables, which hands it the same draws as the first target - the comonotonicity this method exists to prevent, reintroduced with no symptom. It now raises. Today's call sites all pass post-preprocessing names, so this is about the next refactor, not current behaviour. Seeds were validated lazily inside _seed_for_variable, so QRF(seed=-5) constructed fine and only failed part-way through fit, where the blanket handler rewrapped the ValueError as RuntimeError. Validation now happens in __init__, bools included, and the test asserts ValueError at construction rather than RuntimeError at fit. Also drops the docs reformatting, which belongs to #218 alone. --- .../cross-validation.md | 12 +++++------ docs/imputation-benchmarking/preprocessing.md | 8 ++++---- .../imputation-benchmarking/visualizations.md | 8 ++++++-- docs/models/imputer/implement-new-model.md | 4 +++- docs/use_cases/index.md | 12 +++++------ microimpute/models/qrf.py | 20 +++++++++++++++---- tests/test_models/test_qrf.py | 17 +++++++++++----- 7 files changed, 53 insertions(+), 28 deletions(-) diff --git a/docs/imputation-benchmarking/cross-validation.md b/docs/imputation-benchmarking/cross-validation.md index c1b6f93e..8da745ff 100644 --- a/docs/imputation-benchmarking/cross-validation.md +++ b/docs/imputation-benchmarking/cross-validation.md @@ -41,23 +41,23 @@ Returns a dictionary containing separate results for each metric type: ```python { "quantile_loss": { - "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (mean across folds) + "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (mean across folds) "results_std": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (std across folds) "mean_train": float, "mean_test": float, "std_train": float, "std_test": float, - "variables": List[str], # numerical variables evaluated + "variables": List[str] # numerical variables evaluated }, "log_loss": { - "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles + "results": pd.DataFrame, # rows: ["train", "test"], cols: quantiles "results_std": pd.DataFrame, # rows: ["train", "test"], cols: quantiles (std across folds) "mean_train": float, "mean_test": float, "std_train": float, "std_test": float, - "variables": List[str], # categorical variables evaluated - }, + "variables": List[str] # categorical variables evaluated + } } ``` @@ -77,7 +77,7 @@ results = cross_validate_model( data=diabetes_df, predictors=["age", "sex", "bmi", "bp"], imputed_variables=["s1", "s4"], - n_splits=5, + n_splits=5 ) # Check performance for numerical variables diff --git a/docs/imputation-benchmarking/preprocessing.md b/docs/imputation-benchmarking/preprocessing.md index 05693943..736d73ed 100644 --- a/docs/imputation-benchmarking/preprocessing.md +++ b/docs/imputation-benchmarking/preprocessing.md @@ -117,10 +117,10 @@ result = autoimpute( predictors=["age", "education"], imputed_variables=["income", "wealth"], preprocessing={ - "income": "log", # Log transform (positive values only) - "wealth": "asinh", # Asinh transform (handles zeros/negatives) - "age": "normalize", # Z-score normalization - }, + "income": "log", # Log transform (positive values only) + "wealth": "asinh", # Asinh transform (handles zeros/negatives) + "age": "normalize" # Z-score normalization + } ) ``` diff --git a/docs/imputation-benchmarking/visualizations.md b/docs/imputation-benchmarking/visualizations.md index fe8f87b8..21dde021 100644 --- a/docs/imputation-benchmarking/visualizations.md +++ b/docs/imputation-benchmarking/visualizations.md @@ -83,7 +83,11 @@ comparison_viz = method_comparison_results( ) # Generate plot -fig = comparison_viz.plot(title="Method comparison", show_mean=True, plot_type="bar") +fig = comparison_viz.plot( + title="Method comparison", + show_mean=True, + plot_type="bar" +) fig.show() # Get summary statistics @@ -161,7 +165,7 @@ perf_viz = model_performance_results( results=cv_results, model_name="QRF", method_name="Cross-validation", - metric="quantile_loss", + metric="quantile_loss" ) fig = perf_viz.plot(title="QRF performance") diff --git a/docs/models/imputer/implement-new-model.md b/docs/models/imputer/implement-new-model.md index 794d6193..edf21254 100644 --- a/docs/models/imputer/implement-new-model.md +++ b/docs/models/imputer/implement-new-model.md @@ -73,7 +73,9 @@ class NewModelResults(ImputerResults): except Exception as e: self.logger.error(f"Error during Model prediction: {str(e)}") - raise RuntimeError(f"Failed to predict with Model: {str(e)}") from e + raise RuntimeError( + f"Failed to predict with Model: {str(e)}" + ) from e ``` ## Implementing the main model class diff --git a/docs/use_cases/index.md b/docs/use_cases/index.md index 2c9ff07c..32c7af96 100644 --- a/docs/use_cases/index.md +++ b/docs/use_cases/index.md @@ -23,7 +23,7 @@ Before imputation, make sure both datasets have compatible variables. Identify c ```python # Identify common variables -common_variables = ["age", "income", "education", "marital_status", "region"] +common_variables = ['age', 'income', 'education', 'marital_status', 'region'] # Ensure variable formats match (example: education coding) education_mapping = { @@ -31,19 +31,19 @@ education_mapping = { 2: "high_school", 3: "some_college", 4: "bachelor", - 5: "graduate", + 5: "graduate" } # Apply standardization to both datasets for dataset in [scf_data, cps_data]: - dataset["education"] = dataset["education"].map(education_mapping) + dataset['education'] = dataset['education'].map(education_mapping) # Convert income to same units (thousands) - if "income" in dataset.columns: - dataset["income"] = dataset["income"] / 1000 + if 'income' in dataset.columns: + dataset['income'] = dataset['income'] / 1000 # Identify target variable in donor dataset -target_variable = ["networth"] +target_variable = ['networth'] ``` ## Performing imputation diff --git a/microimpute/models/qrf.py b/microimpute/models/qrf.py index 76bc4768..9e5560d3 100644 --- a/microimpute/models/qrf.py +++ b/microimpute/models/qrf.py @@ -623,6 +623,14 @@ def __init__( seed derived from it, so variables imputed together draw independently. Pass None for non-reproducible draws. """ + if seed is not None and ( + isinstance(seed, bool) + or not isinstance(seed, (int, np.integer)) + or not 0 <= seed < 2**32 + ): + raise ValueError( + f"seed must be an integer from 0 to 2**32 - 1, or None, got {seed!r}" + ) super().__init__(log_level=log_level, seed=seed) self.models = {} self.log_level = log_level @@ -711,12 +719,16 @@ def _seed_for_variable(self, variable: str) -> Optional[int]: """ if self.seed is None: return None - if not isinstance(self.seed, (int, np.integer)) or not 0 <= self.seed < 2**32: - raise ValueError("seed must be an integer from 0 to 2**32 - 1 or None") try: variable_offset = (self.imputed_variables or []).index(variable) - except ValueError: - variable_offset = 0 + except ValueError as error: + # Falling back to offset 0 would hand this variable the same draws + # as the first target, which is the comonotonicity this method + # exists to prevent - and it would do so silently. + raise ValueError( + f"Cannot derive a seed for {variable!r}: it is not among the " + f"imputed variables {list(self.imputed_variables or [])}." + ) from error return (int(self.seed) + variable_offset) % 2**32 def _create_model_for_variable(self, variable: str, **kwargs) -> Any: diff --git a/tests/test_models/test_qrf.py b/tests/test_models/test_qrf.py index cd203360..50d3f31b 100644 --- a/tests/test_models/test_qrf.py +++ b/tests/test_models/test_qrf.py @@ -1738,11 +1738,18 @@ def test_qrf_child_seeds_preserve_existing_seed_values(seed) -> None: @pytest.mark.parametrize("seed", [-1, 2**32, 1.5]) def test_qrf_invalid_base_seed_is_not_normalized(seed) -> None: - """Wrapping child seeds must not silently accept invalid sklearn base seeds.""" - rng = np.random.default_rng(3) - data = pd.DataFrame({"x": rng.normal(size=30), "y": rng.normal(size=30)}) - with pytest.raises(RuntimeError): - QRF(seed=seed).fit(data, ["x"], ["y"], n_estimators=5) + """An invalid seed fails at construction, not part-way through a fit.""" + with pytest.raises(ValueError, match="seed must be an integer"): + QRF(seed=seed) + + +def test_qrf_seed_for_unknown_variable_raises() -> None: + """Falling back to offset 0 would silently recreate comonotonicity.""" + model = QRF(seed=100) + model.imputed_variables = ["a", "b"] + assert [model._seed_for_variable(v) for v in ["a", "b"]] == [100, 101] + with pytest.raises(ValueError, match="not among the imputed variables"): + model._seed_for_variable("z") @pytest.mark.parametrize("target_type", ["numeric", "boolean"])