diff --git a/CHANGELOG.md b/CHANGELOG.md index b2b686b..f646299 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,14 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) mkdocstrings-generated API reference. ### Changed +- **Breaking:** `FiscalYearGroupedSplitter(drop_repeat_donors=...)` now + defaults to `True`, as the `DeprecationWarning` in 0.7.0 and 0.7.1 said it + would. Each test fold drops donors already seen in its training rows, which + is the safe default for a static per-donor label. It needs `groups` as + `(n_samples, 2)` (fiscal year, donor id); code that passes fiscal years alone + now raises a `ValueError` that names both fixes. Pass + `drop_repeat_donors=False` for a time-varying target to keep the 0.7.x + behaviour. The two leakage experiment scripts now pass it explicitly. - The docs homepage hero no longer uses an all-caps eyebrow label or a gradient-clipped headline; it's now a two-column layout with the headline beside a real ranked-donor ledger table showing what @@ -121,6 +129,12 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) Closes #195. ### Removed +- **Breaking:** `philanthropy.utils.make_donor_dataset`, deprecated since + 0.7.0. Import it from `philanthropy.datasets`. +- **Breaking:** `WealthScreeningImputerKNN(group_col_idx=...)` and its + `group_imputers_` attribute, deprecated since 0.7.0. Per-group and global KNN + fits were measured bit-identical, so the parameter bought nothing; passing it + is now a `TypeError`. `tests/test_knn_group_stratification.py` goes with it. - Deleted `philanthropy/preprocessing/_solicitation_window.py`, a dead module nothing imported. The deprecated `SolicitationWindowTransformer` alias it held was already served by the subpackage's PEP 562 module-level `__getattr__`, so diff --git a/docs/reference/index.md b/docs/reference/index.md index 40c4261..8d6b2f6 100755 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -127,11 +127,16 @@ freeze them: `metrics.scoring` → `metrics._scoring`, subpackage (`from philanthropy.metrics import ...`), as the documented examples always have, and nothing breaks. -### Live in 0.7.0, removed in 0.8.0 +### Removed or changed in 0.8.0 -| Deprecated | Use instead | +If you are upgrading from 0.7.x, these went after two published releases of +`DeprecationWarning`: + +| Removed or changed | Use instead | |---|---| +| `philanthropy.utils.make_donor_dataset` | `philanthropy.datasets.make_donor_dataset` | | `WealthScreeningImputerKNN(group_col_idx=...)` | nothing; see below | +| `FiscalYearGroupedSplitter(drop_repeat_donors=...)` default is now `True` | pass donor ids, or `drop_repeat_donors=False` for a time-varying target | `group_col_idx` has no replacement because there is nothing to replace. It was documented for a long time as stratifying KNN imputation per group "improving @@ -141,11 +146,20 @@ pools and on five Python versions in CI, per-group and global KNN imputation produce **bit-identical** output (`50263.48615163204` both ways). A donor's nearest neighbours by feature distance almost always share their group already, and `KNNImputer` weights distance by column magnitude, so a 0/1 group flag -barely registers. +barely registers. Passing it is now a `TypeError`. If you need per-group behaviour, split the frame by group and fit one imputer per part. That is explicit, and it costs nothing that the parameter was buying. +`FiscalYearGroupedSplitter` now drops, from each test fold, donors already seen +in that fold's training rows. That is the safe choice for a static per-donor +label such as `is_major_donor`, which a model can otherwise memorise from the +donor's earlier years. It needs `groups` as `(n_samples, 2)`: fiscal year, then +donor id. Code that passes fiscal years alone now raises a `ValueError` naming +both fixes. For a time-varying target such as "gave next year", where the same +donor correctly appears in both folds, pass `drop_repeat_donors=False`, which +gives exactly the 0.7.x behaviour. + ### Live in 0.7.1, removed in 0.9.0 | Deprecated | Use instead | diff --git a/philanthropy/model_selection/_temporal_donor_splitter.py b/philanthropy/model_selection/_temporal_donor_splitter.py index 91d362d..a8ee440 100755 --- a/philanthropy/model_selection/_temporal_donor_splitter.py +++ b/philanthropy/model_selection/_temporal_donor_splitter.py @@ -72,26 +72,26 @@ class FiscalYearGroupedSplitter(BaseCrossValidator): the fiscal year immediately before the test year is withheld from training (useful when gift officers use current-year pipeline intelligence that would not have been available historically). - drop_repeat_donors : bool, default=False - .. deprecated:: 0.7.0 - Leaving ``drop_repeat_donors`` at its default emits a - ``DeprecationWarning``. The default changes to ``True`` in 0.8.0. - Pass ``drop_repeat_donors=False`` explicitly to silence this warning - and keep the current behaviour. + drop_repeat_donors : bool, default=True + .. versionchanged:: 0.8.0 + The default changed from ``False`` to ``True``, after 0.7.0 and + 0.7.1 warned about it. Code that relied on the old default and + passes one-dimensional ``groups`` now raises a ``ValueError`` + asking for either donor ids or ``drop_repeat_donors=False``. Whether to remove from each test fold any donor who already appears in that fold's training rows. - Leave this ``False`` for a **time-varying** target such as "did this - donor give in FY22?". There, a donor appearing in both folds is correct: - the training rows precede the test rows in time, which is the point of - walk-forward evaluation. - - Set it ``True`` for a **static per-donor** label such as + Keep it ``True`` for a **static per-donor** label such as ``is_major_donor``, where the same answer is attached to every one of that donor's rows, so the model can memorise it from the donor's earlier years. That is the leakage described under "What this does not prevent" - below. + below, and the reason this is the default. + + Set it ``False`` for a **time-varying** target such as "did this donor + give in FY22?". There, a donor appearing in both folds is correct: the + training rows precede the test rows in time, which is the point of + walk-forward evaluation. When ``True``, ``groups`` must be two-dimensional with shape ``(n_samples, 2)``: column 0 the fiscal year, column 1 the donor @@ -166,8 +166,8 @@ class FiscalYearGroupedSplitter(BaseCrossValidator): the same answer is attached to every one of that donor's rows and the model can memorise it from the training years. - For that case, set ``drop_repeat_donors=True`` and pass ``groups`` as - ``(n_samples, 2)`` with the donor identifier in column 1. Each test fold then + For that case, keep the default ``drop_repeat_donors=True`` and pass + ``groups`` as ``(n_samples, 2)`` with the donor identifier in column 1. Each test fold then excludes donors already present in its training rows. Aggregating to one row per donor and using a grouped holdout remains the cleaner option when the label has no time dimension at all. @@ -185,22 +185,12 @@ def __init__( self, n_splits: int = 5, gap_years: int = 0, - drop_repeat_donors: bool | str = "warn", + drop_repeat_donors: bool = True, ) -> None: # MUST call super().__init__() for BaseCrossValidator compat. self.n_splits = n_splits self.gap_years = gap_years self.drop_repeat_donors = drop_repeat_donors - - if self.drop_repeat_donors == "warn": - warnings.warn( - "The FiscalYearGroupedSplitter(drop_repeat_donors=...) default " - "of False is deprecated and allows repeat donors across train " - "and test folds. This default will change to True in 0.8.0. Pass " - "drop_repeat_donors=False explicitly to silence this warning.", - DeprecationWarning, - stacklevel=2, - ) # ------------------------------------------------------------------ # Required abstract-method implementations @@ -270,7 +260,7 @@ def split( If ``drop_repeat_donors=True`` empties a test fold entirely. """ requested_splits, gap_years = self._validate_params() - drop_repeat = False if self.drop_repeat_donors == "warn" else bool(self.drop_repeat_donors) + drop_repeat = bool(self.drop_repeat_donors) if groups is None: raise ValueError( @@ -284,9 +274,12 @@ def split( if drop_repeat: if groups_arr.ndim != 2 or groups_arr.shape[1] != 2: raise ValueError( - "drop_repeat_donors=True requires `groups` with shape " - "(n_samples, 2): column 0 the fiscal year, column 1 the " - f"donor identifier. Got shape {groups_arr.shape}." + "drop_repeat_donors=True (the default since 0.8.0) requires " + "`groups` with shape (n_samples, 2): column 0 the fiscal " + "year, column 1 the donor identifier. Got shape " + f"{groups_arr.shape}. For a time-varying target such as " + "'gave next year', where a donor belongs in both folds, " + "pass drop_repeat_donors=False with fiscal years alone." ) donor_ids = groups_arr[:, 1] groups_arr = groups_arr[:, 0] @@ -407,7 +400,7 @@ def get_n_splits( n_splits, gap_years = self._validate_params() if groups is not None: groups = np.asarray(groups) - drop_repeat = False if self.drop_repeat_donors == "warn" else bool(self.drop_repeat_donors) + drop_repeat = bool(self.drop_repeat_donors) if drop_repeat and groups.ndim == 2 and groups.shape[1] == 2: groups = groups[:, 0] unique_fy = np.unique(groups) diff --git a/philanthropy/preprocessing/_share_of_wallet.py b/philanthropy/preprocessing/_share_of_wallet.py index 6289216..ac83e5b 100755 --- a/philanthropy/preprocessing/_share_of_wallet.py +++ b/philanthropy/preprocessing/_share_of_wallet.py @@ -87,44 +87,6 @@ class WealthScreeningImputerKNN(TransformerMixin, BaseEstimator): Append a binary ``__was_missing`` column for each imputed wealth column. Strongly recommended: absence of vendor records itself carries predictive signal. - group_col_idx : int or None, default=None - .. deprecated:: 0.7.0 - Passing ``group_col_idx`` emits a ``DeprecationWarning`` and the - parameter is removed in 0.8.0. It still works meanwhile; there is no - replacement, because there is nothing to replace. - - The reason is measurement, not tidiness. Across several synthetic - two-group pools, and on five Python versions in CI, the grouped and - global fits produce **bit-identical** output: 50263.48615163204 both - ways. That is not a near miss. A donor's nearest neighbours by - feature distance almost always share their group already, so - restricting the fit to the group changes nothing, and - :class:`~sklearn.impute.KNNImputer` weights distance by column - magnitude, so a 0/1 group flag contributes almost nothing on its own. - The parameter costs a per-group imputer, three fallback paths and a - documented contract, and buys no measurable accuracy. If you need - per-group behaviour, split the frame by group and fit one imputer per - part, which is explicit and costs nothing here. - - Column index of a group variable (for example a zip code encoded as an - int) to stratify KNN imputation. When set and ``strategy="knn"``, a - separate :class:`~sklearn.impute.KNNImputer` is fitted per group, so a - donor's missing wealth is filled from neighbours inside their own group - rather than from the whole database. Ignored for the other strategies, - which are columnwise statistics with no notion of a neighbourhood. - - Two fallbacks, both frozen at :meth:`fit` time so nothing is learned at - transform time: - - * A group with fewer than ``n_neighbors + 1`` training rows gets no - imputer of its own, because KNN over too few neighbours is worse than - the global fit. Its rows use the global imputer. - * A group value **not seen during fit**, or a row whose group value is - missing, also uses the global imputer. This is the leakage-safe - choice: the alternative is fitting on the data being transformed. - - The global imputer is always fitted, so the output is never ``NaN`` - regardless of grouping. Attributes ---------- @@ -132,11 +94,6 @@ class WealthScreeningImputerKNN(TransformerMixin, BaseEstimator): Wealth columns that were actually present in ``X`` at fit time. fill_values_ : dict of {str: float} Fill statistics (only populated for non-KNN strategies). - group_imputers_ : dict of {float: tuple} - Maps each group value that qualified for its own imputer to - ``(fitted KNNImputer, boolean mask of columns entirely missing within - that group)``. Empty when ``group_col_idx`` is ``None`` or ``strategy`` - is not ``"knn"``. knn_imputer_ : KNNImputer or None The fitted :class:`~sklearn.impute.KNNImputer` instance (only populated for ``strategy="knn"``). @@ -169,13 +126,11 @@ def __init__( strategy: Literal["median", "mean", "zero", "knn"] = "knn", n_neighbors: int = 5, add_indicator: bool = True, - group_col_idx: Optional[int] = None, ) -> None: self.wealth_cols = wealth_cols self.strategy = strategy self.n_neighbors = n_neighbors self.add_indicator = add_indicator - self.group_col_idx = group_col_idx def _resolve_cols(self, input_cols: list[str]) -> list[str]: if self.wealth_cols is not None: @@ -195,19 +150,6 @@ def fit(self: _SelfK, X: Any, y: Any = None) -> _SelfK: ------- self : WealthScreeningImputerKNN """ - if self.group_col_idx is not None: - warnings.warn( - "WealthScreeningImputerKNN(group_col_idx=...) is deprecated " - "since 0.7.0 and will be removed in 0.8.0. It has no " - "replacement: measured across several synthetic pools and five " - "Python versions in CI, per-group and global KNN imputation " - "produce bit-identical output, so the parameter buys no " - "accuracy. Split the frame by group and fit one imputer per part " - "if you need that behaviour.", - DeprecationWarning, - stacklevel=2, - ) - if self.strategy not in self._VALID_STRATEGIES: raise ValueError( f"`strategy` must be one of {sorted(self._VALID_STRATEGIES)}, " @@ -248,8 +190,6 @@ def fit(self: _SelfK, X: Any, y: Any = None) -> _SelfK: if self.strategy == "knn": # Fit KNNImputer on ALL columns (preserves inter-column structure). - # Always fitted, even when grouping: it is the fallback for small and - # unseen groups, and it is what guarantees the output has no NaN. self.knn_imputer_: Optional[KNNImputer] = KNNImputer( n_neighbors=self.n_neighbors, weights="distance", @@ -257,42 +197,8 @@ def fit(self: _SelfK, X: Any, y: Any = None) -> _SelfK: ) self.knn_imputer_.fit(X_arr) self.fill_values_: dict[str, float] = {} - self.group_imputers_: dict = {} - if self.group_col_idx is not None: - gidx = int(self.group_col_idx) - if not -X_arr.shape[1] <= gidx < X_arr.shape[1]: - raise ValueError( - f"`group_col_idx` {gidx} is out of range for X with " - f"{X_arr.shape[1]} columns." - ) - groups = X_arr[:, gidx] - # A group needs more rows than neighbours for KNN to mean - # anything; smaller groups deliberately get no imputer and fall - # back to the global one. - min_rows = self.n_neighbors + 1 - for value in np.unique(groups[~np.isnan(groups)]): - rows = groups == value - if int(rows.sum()) < min_rows: - continue - sub_imputer = KNNImputer( - n_neighbors=self.n_neighbors, - weights="distance", - keep_empty_features=True, - ) - sub_imputer.fit(X_arr[rows]) - # Columns entirely missing WITHIN this group. KNNImputer with - # keep_empty_features=True fills those with a hard 0.0, not - # NaN, so a NaN check at transform time cannot see them. For a - # wealth column, 0.0 reads as "no capacity", which is a - # materially wrong answer rather than a missing one. Record - # them so transform defers to the global imputer instead. - self.group_imputers_[float(value)] = ( - sub_imputer, - np.isnan(X_arr[rows]).all(axis=0), - ) else: self.knn_imputer_ = None - self.group_imputers_ = {} fills: dict[str, float] = {} for col in self.imputed_cols_: idx = col_indices[col] @@ -353,23 +259,7 @@ def transform(self, X: Any, y: Any = None) -> np.ndarray: indicators.append(np.isnan(X_arr[:, idx]).astype(np.float64).reshape(-1, 1)) if self.strategy == "knn" and self.knn_imputer_ is not None: - # Global result first: this is the fallback for small groups, unseen - # groups and missing group labels, and it guarantees no NaN survives. X_out = self.knn_imputer_.transform(X_arr) - group_imputers = getattr(self, "group_imputers_", {}) - if group_imputers and self.group_col_idx is not None: - groups = X_arr[:, int(self.group_col_idx)] - for value, (sub_imputer, empty_cols) in group_imputers.items(): - rows = groups == value - if not rows.any(): - continue - X_group = sub_imputer.transform(X_arr[rows]) - # Prefer the group-local value, except where it cannot be - # trusted: a NaN it failed to fill, or a column entirely - # missing inside this group, where KNNImputer returns a hard - # 0.0 that would read as real data. - reject = np.isnan(X_group) | empty_cols[None, :] - X_out[rows] = np.where(reject, X_out[rows], X_group) else: X_out = X_arr.copy() for col in self.imputed_cols_: diff --git a/philanthropy/utils/__init__.py b/philanthropy/utils/__init__.py index b117626..22f8764 100755 --- a/philanthropy/utils/__init__.py +++ b/philanthropy/utils/__init__.py @@ -1,29 +1,9 @@ """ philanthropy.utils ================== -Generic helpers: model persistence and deprecated aliases. +Generic helpers: model persistence. """ -import warnings -from typing import Any - -import pandas as pd - from ._persistence import save_model, load_model - -def make_donor_dataset(*args: Any, **kwargs: Any) -> pd.DataFrame: - """Deprecated alias for :func:`philanthropy.datasets.make_donor_dataset`.""" - warnings.warn( - "philanthropy.utils.make_donor_dataset is deprecated and will be " - "removed in 0.8.0; import make_donor_dataset from philanthropy.datasets " - "instead.", - DeprecationWarning, - stacklevel=2, - ) - from ..datasets import make_donor_dataset as _make_donor_dataset - - return _make_donor_dataset(*args, **kwargs) - - -__all__ = ["make_donor_dataset", "save_model", "load_model"] +__all__ = ["save_model", "load_model"] diff --git a/scripts/leakage_experiment.py b/scripts/leakage_experiment.py index c31b1b2..f0e82d0 100644 --- a/scripts/leakage_experiment.py +++ b/scripts/leakage_experiment.py @@ -116,8 +116,8 @@ def main(): as_of, whole = _panel(seed) truth.append(_true_future(as_of)) random_cv.append(_cv(as_of, StratifiedKFold(5, shuffle=True, random_state=0), False)) - walk_cv.append(_cv(as_of, FiscalYearGroupedSplitter(n_splits=3), True)) - whole_cv.append(_cv(whole, FiscalYearGroupedSplitter(n_splits=3), True)) + walk_cv.append(_cv(as_of, FiscalYearGroupedSplitter(n_splits=3, drop_repeat_donors=False), True)) + whole_cv.append(_cv(whole, FiscalYearGroupedSplitter(n_splits=3, drop_repeat_donors=False), True)) print(f"Donor-year panel: {N_DONORS} donors x {len(YEARS) - 1} panel years, " f"label = gave in the following year.") diff --git a/scripts/real_data_leakage_experiment.py b/scripts/real_data_leakage_experiment.py index 5226744..6a0d23e 100644 --- a/scripts/real_data_leakage_experiment.py +++ b/scripts/real_data_leakage_experiment.py @@ -147,8 +147,8 @@ def main(): random_cv.append( _cv(as_of, StratifiedKFold(5, shuffle=True, random_state=seed), False, seed) ) - walk_cv.append(_cv(as_of, FiscalYearGroupedSplitter(n_splits=3), True, seed)) - whole_cv.append(_cv(whole, FiscalYearGroupedSplitter(n_splits=3), True, seed)) + walk_cv.append(_cv(as_of, FiscalYearGroupedSplitter(n_splits=3, drop_repeat_donors=False), True, seed)) + whole_cv.append(_cv(whole, FiscalYearGroupedSplitter(n_splits=3, drop_repeat_donors=False), True, seed)) print(f"KDD Cup 1998 donor-period panel: {as_of['donor'].nunique()} donors x " f"{n_periods} promotion periods, label = gave at the following period " diff --git a/tests/test_deprecations.py b/tests/test_deprecations.py index 167701a..59a873e 100644 --- a/tests/test_deprecations.py +++ b/tests/test_deprecations.py @@ -12,18 +12,11 @@ import numpy as np import pytest -from philanthropy import preprocessing, utils -from philanthropy.preprocessing import ShareOfWalletScorer, WealthScreeningImputerKNN +from philanthropy import preprocessing +from philanthropy.preprocessing import ShareOfWalletScorer # (id, removed_in, callable that should emit exactly one DeprecationWarning) DEPRECATIONS = [ - ( - "WealthScreeningImputerKNN.group_col_idx", - "0.8.0", - lambda: WealthScreeningImputerKNN( - strategy="knn", n_neighbors=3, add_indicator=False, group_col_idx=1 - ).fit(_two_group_X()), - ), ( "ShareOfWalletScorer.get_legacy_feature_names_out", "0.9.0", @@ -31,11 +24,6 @@ .fit(_two_group_X()) .get_legacy_feature_names_out(), ), - ( - "utils.make_donor_dataset", - "0.8.0", - lambda: utils.make_donor_dataset(n_donors=5, random_state=0), - ), ( "preprocessing.SolicitationWindowTransformer", "1.0.0", @@ -43,11 +31,6 @@ # subpackage's PEP 562 __getattr__ so it stays the canonical class. lambda: preprocessing.SolicitationWindowTransformer, ), - ( - "FiscalYearGroupedSplitter.drop_repeat_donors", - "0.8.0", - lambda: __import__("philanthropy.model_selection").model_selection.FiscalYearGroupedSplitter(), - ), ] @@ -81,13 +64,11 @@ def test_shim_still_works(dep_id, removed_in, trigger): trigger() -def test_no_warning_when_the_deprecated_parameter_is_untouched(): +def test_no_warning_when_the_deprecated_path_is_untouched(): # The other half of the contract: callers who never used it see nothing. with warnings.catch_warnings(record=True) as caught: warnings.simplefilter("always") - WealthScreeningImputerKNN( - strategy="knn", n_neighbors=3, add_indicator=False - ).fit(_two_group_X()) + ShareOfWalletScorer().fit(_two_group_X()).get_feature_names_out() assert not [w for w in caught if issubclass(w.category, DeprecationWarning)] diff --git a/tests/test_documented_contracts.py b/tests/test_documented_contracts.py index 188e436..78bb34b 100644 --- a/tests/test_documented_contracts.py +++ b/tests/test_documented_contracts.py @@ -44,37 +44,13 @@ def test_fiscal_year_transformer_all_nan_when_date_col_absent(): assert np.isnan(out).all() -@pytest.mark.filterwarnings( - "ignore:WealthScreeningImputerKNN\\(group_col_idx:DeprecationWarning" -) -def test_group_col_idx_is_wired_up_not_ignored(): - # This test previously asserted the opposite, locking in "group_col_idx is - # stored and never read" from when the docstring said "ignored". - # - # It deliberately does NOT assert that grouping changes the imputed values. - # Measured across several synthetic setups, that difference is small and not - # reliably reproducible: a donor's nearest neighbours by feature distance - # usually share their group already, so the grouped and global fits often - # agree exactly. What is reliable, and what this locks in, is that the - # parameter is honoured rather than discarded. - rng = np.random.default_rng(0) - n = 40 - X = np.column_stack([ - np.r_[rng.normal(5e4, 2e3, n), rng.normal(5e6, 2e5, n)], - np.r_[np.zeros(n), np.ones(n)], - ]) - X[0, 0] = np.nan - X[n, 0] = np.nan - - kwargs = dict(strategy="knn", n_neighbors=5, add_indicator=False) - model = WealthScreeningImputerKNN(group_col_idx=1, **kwargs).fit(X) - - # A per-group imputer exists for each qualifying group, which is the thing - # that was previously absent entirely. - assert set(model.group_imputers_) == {0.0, 1.0} - out = model.transform(X) - assert not np.isnan(out).any() - assert out[0, 0] < 1e5 and out[n, 0] > 1e6 +def test_group_col_idx_is_removed(): + # Deprecated in 0.7.0 and removed in 0.8.0: per-group and global KNN fits + # were measured bit-identical, so the parameter bought nothing. Passing it + # is now the usual unexpected-keyword TypeError, not a silent no-op. + with pytest.raises(TypeError, match="group_col_idx"): + WealthScreeningImputerKNN(group_col_idx=1) + assert "group_col_idx" not in WealthScreeningImputerKNN().get_params() def test_days_since_last_discharge_is_float_and_carries_nan(): diff --git a/tests/test_knn_group_stratification.py b/tests/test_knn_group_stratification.py deleted file mode 100644 index b4e65c9..0000000 --- a/tests/test_knn_group_stratification.py +++ /dev/null @@ -1,155 +0,0 @@ -"""Per-group KNN imputation on WealthScreeningImputerKNN. - -`group_col_idx` was documented, stored and never read. Now it stratifies the -KNN fit, so a donor's missing wealth is filled from neighbours inside their own -group rather than from the whole database. -""" - -import numpy as np -import pandas as pd -import pytest - -from philanthropy.preprocessing import WealthScreeningImputerKNN - -pytestmark = pytest.mark.filterwarnings( - # This whole file exercises the deprecated `group_col_idx`, so the shim - # warning is expected here rather than a signal. Scoped to that message so an - # unrelated DeprecationWarning still surfaces. When the parameter goes in - # 0.8.0, this file goes with it. - "ignore:WealthScreeningImputerKNN\\(group_col_idx:DeprecationWarning" -) - -KW = dict(strategy="knn", n_neighbors=5, add_indicator=False) - - -def _two_group_pool(seed=0, n=40): - """Two groups an order of magnitude apart, one missing wealth in each.""" - rng = np.random.default_rng(seed) - lo = np.column_stack([rng.normal(50_000, 3_000, n), np.zeros(n)]) - hi = np.column_stack([rng.normal(5_000_000, 200_000, n), np.ones(n)]) - X = np.vstack([lo, hi]) - X[0, 0] = np.nan # low group, missing - X[n, 0] = np.nan # high group, missing - return X, n - - -def test_grouped_fill_comes_from_inside_the_group(): - # Deliberately does NOT assert that grouping changes the value. - # - # An earlier version did, and CI failed it: on other numpy/sklearn versions - # the grouped and global fills came out bit-identical - # (50263.48615163204 both ways). That is not flakiness, it is the real - # behaviour: a donor's nearest neighbours by feature distance usually share - # their group already, so restricting the fit to the group changes nothing. - # An assertion that grouping moves the answer is therefore not a contract - # this parameter can honour, and asserting it anyway just encodes a wish. - # - # What is reliable, and what this checks: the fill lands in the right group's - # range and no NaN survives. The parameter being honoured at all is asserted - # via group_imputers_ below and in tests/test_documented_contracts.py. - X, n = _two_group_pool() - grouped = WealthScreeningImputerKNN(group_col_idx=1, **KW).fit(X) - out = grouped.transform(X) - - assert set(grouped.group_imputers_) == {0.0, 1.0} - assert out[0, 0] < 100_000 - assert out[n, 0] > 1_000_000 - assert not np.isnan(out).any() - - -def test_column_all_missing_within_a_group_does_not_become_zero(): - # The bug this guard exists for. KNNImputer(keep_empty_features=True) fills a - # wholly-missing column with a hard 0.0 rather than NaN, so a NaN check at - # transform time cannot see it. For a wealth column, 0.0 reads as "no - # capacity": a materially wrong number, silently, for every donor in that - # group. Those columns must defer to the global imputer. - n = 20 - X = np.vstack([ - np.column_stack([np.full(n, np.nan), np.linspace(1e3, 2e3, n), np.zeros(n)]), - np.column_stack([np.linspace(4e6, 6e6, n), np.linspace(5e5, 7e5, n), np.ones(n)]), - ]) - grouped = WealthScreeningImputerKNN(group_col_idx=2, **KW).fit(X).transform(X) - plain = WealthScreeningImputerKNN(**KW).fit(X).transform(X) - - assert not np.any(grouped[:n, 0] == 0.0) - np.testing.assert_allclose(grouped[:n, 0], plain[:n, 0]) - - -def test_grouping_is_wired_through_to_transform(): - # This asserted `not np.allclose(grouped, plain)` and was wrong to: on some - # numpy/sklearn versions the two agree exactly, because the global KNN fit - # already draws its neighbours from within the group. The observable, - # reliable contract is that a per-group imputer is fitted and used. - X, _ = _two_group_pool() - model = WealthScreeningImputerKNN(group_col_idx=1, **KW).fit(X) - assert set(model.group_imputers_) == {0.0, 1.0} - for _imputer, empty_cols in model.group_imputers_.values(): - assert empty_cols.shape == (X.shape[1],) - assert not np.isnan(model.transform(X)).any() - - -def test_group_smaller_than_n_neighbors_falls_back_to_global(): - # KNN over fewer neighbours than requested is worse than the global fit, so - # a small group deliberately gets no imputer of its own. - X, _ = _two_group_pool() - tiny = np.column_stack([np.full(3, 9e6), np.full(3, 2.0)]) - tiny[-1, 0] = np.nan - X = np.vstack([X, tiny]) - - model = WealthScreeningImputerKNN(group_col_idx=1, **KW).fit(X) - assert 2.0 not in model.group_imputers_ # too small, excluded - assert {0.0, 1.0} <= set(model.group_imputers_) - assert not np.isnan(model.transform(X)).any() # still imputed, via global - - -def test_group_unseen_at_fit_uses_the_global_imputer(): - # The leakage-safe choice. The alternative is fitting at transform time. - X, _ = _two_group_pool() - model = WealthScreeningImputerKNN(group_col_idx=1, **KW).fit(X) - unseen = np.array([[np.nan, 99.0]]) - out = model.transform(unseen) - assert np.isfinite(out[0, 0]) - - -def test_missing_group_label_uses_the_global_imputer(): - X, _ = _two_group_pool() - model = WealthScreeningImputerKNN(group_col_idx=1, **KW).fit(X) - out = model.transform(np.array([[np.nan, np.nan]])) - assert np.isfinite(out[0, 0]) - - -def test_transform_is_idempotent_and_fits_nothing(): - # The leakage contract: repeated transforms must agree, and transforming a - # batch must not change what a later transform of the same rows returns. - X, _ = _two_group_pool() - model = WealthScreeningImputerKNN(group_col_idx=1, **KW).fit(X) - first = model.transform(X) - fitted_groups = set(model.group_imputers_) # snapshot the keys themselves - assert fitted_groups # and it is not vacuously empty - model.transform(np.vstack([X, X])) # a bigger, different batch - np.testing.assert_array_equal(first, model.transform(X)) - assert set(model.group_imputers_) == fitted_groups - - -@pytest.mark.parametrize("strategy", ["median", "mean", "zero"]) -def test_ignored_for_non_knn_strategies(strategy): - X, _ = _two_group_pool() - df = pd.DataFrame(X, columns=["net_worth", "zip_group"]) - grouped = WealthScreeningImputerKNN( - strategy=strategy, group_col_idx=1, add_indicator=False - ).fit(df).transform(df) - plain = WealthScreeningImputerKNN( - strategy=strategy, add_indicator=False - ).fit(df).transform(df) - np.testing.assert_array_equal(grouped, plain) - - -@pytest.mark.parametrize("bad", [99, -99]) -def test_out_of_range_group_col_idx_raises(bad): - X, _ = _two_group_pool() - with pytest.raises(ValueError, match="out of range"): - WealthScreeningImputerKNN(group_col_idx=bad, **KW).fit(X) - - -def test_group_col_idx_still_round_trips_through_get_params(): - assert WealthScreeningImputerKNN(group_col_idx=3).get_params()["group_col_idx"] == 3 diff --git a/tests/test_model_selection.py b/tests/test_model_selection.py index cbf2d05..34c21d0 100644 --- a/tests/test_model_selection.py +++ b/tests/test_model_selection.py @@ -109,11 +109,10 @@ def test_not_enough_fiscal_years_names_the_shortfall(): def test_repr_and_get_n_splits_reflect_the_groups(): - with pytest.warns(DeprecationWarning): - splitter = FiscalYearGroupedSplitter(n_splits=2, gap_years=1) + splitter = FiscalYearGroupedSplitter(n_splits=2, gap_years=1) assert repr(splitter) == ( "FiscalYearGroupedSplitter(n_splits=2, gap_years=1, " - "drop_repeat_donors='warn')" + "drop_repeat_donors=True)" ) assert splitter.get_n_splits(groups=_FY_GROUPS) == 2 @@ -182,13 +181,12 @@ def _repeat_donor_panel(): return np.zeros((len(fy), 2)), fy, donor -def test_default_leaves_repeat_donors_in_both_folds(): - # Documented and correct for a time-varying target; this pins the default so - # the new flag cannot quietly become the default later. +def test_false_leaves_repeat_donors_in_both_folds(): + # Documented and correct for a time-varying target. X, fy, donor = _repeat_donor_panel() - with pytest.warns(DeprecationWarning): - for train, test in FiscalYearGroupedSplitter(n_splits=2).split(X, groups=fy): - assert set(donor[train]) & set(donor[test]) == {1, 2, 3} + splitter = FiscalYearGroupedSplitter(n_splits=2, drop_repeat_donors=False) + for train, test in splitter.split(X, groups=fy): + assert set(donor[train]) & set(donor[test]) == {1, 2, 3} def test_drop_repeat_donors_removes_the_overlap(): @@ -231,38 +229,35 @@ def test_drop_repeat_donors_raises_rather_than_silently_dropping_a_fold(): list(splitter.split(np.zeros((4, 2)), groups=groups)) -def test_default_drop_repeat_donors_emits_deprecation_warning(): - with pytest.warns(DeprecationWarning, match="default of False"): - FiscalYearGroupedSplitter() - - -def test_explicit_drop_repeat_donors_false_silences_warning(): +def test_drop_repeat_donors_is_on_by_default(): + # Default flipped from False to True in 0.8.0, after two releases of + # DeprecationWarning. BaseCrossValidator has no get_params, so read the attr. import warnings - with warnings.catch_warnings(record=True) as w: - warnings.simplefilter('always') - FiscalYearGroupedSplitter(drop_repeat_donors=False) - assert not any(issubclass(x.category, DeprecationWarning) for x in w) + with warnings.catch_warnings(): + warnings.simplefilter("error") + splitter = FiscalYearGroupedSplitter() + assert splitter.drop_repeat_donors is True -def test_drop_repeat_donors_is_off_by_default(): - # BaseCrossValidator, not BaseEstimator, so there is no get_params here. - with pytest.warns(DeprecationWarning): - assert FiscalYearGroupedSplitter().drop_repeat_donors == "warn" +def test_default_with_fiscal_years_alone_says_how_to_opt_out(): + # The migration path for code written against the old default: the error + # names both fixes rather than only the shape. + X, fy, _ = _repeat_donor_panel() + with pytest.raises(ValueError, match="drop_repeat_donors=False"): + list(FiscalYearGroupedSplitter(n_splits=2).split(X, groups=fy)) def test_repr_distinguishes_splitters_that_behave_differently(): # This test used to assert the opposite, that drop_repeat_donors was absent # from __repr__. That pinned a defect: two splitters that split differently # printed identically, which is exactly what a repr exists to prevent. - with pytest.warns(DeprecationWarning): - assert "drop_repeat_donors='warn'" in repr(FiscalYearGroupedSplitter()) - assert "drop_repeat_donors=True" in repr( - FiscalYearGroupedSplitter(drop_repeat_donors=True) + assert "drop_repeat_donors=True" in repr(FiscalYearGroupedSplitter()) + assert "drop_repeat_donors=False" in repr( + FiscalYearGroupedSplitter(drop_repeat_donors=False) + ) + assert repr(FiscalYearGroupedSplitter()) != repr( + FiscalYearGroupedSplitter(drop_repeat_donors=False) ) - with pytest.warns(DeprecationWarning): - assert repr(FiscalYearGroupedSplitter()) != repr( - FiscalYearGroupedSplitter(drop_repeat_donors=True) - ) def test_missing_donor_id_is_treated_as_already_seen(): diff --git a/tests/test_utils.py b/tests/test_utils.py index 0826fcd..4604c20 100755 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -2,12 +2,10 @@ tests/test_utils.py """ -import warnings - import pandas as pd import pytest -from philanthropy.utils import make_donor_dataset +from philanthropy.datasets import make_donor_dataset def test_make_donor_dataset_shape(): @@ -34,20 +32,10 @@ def test_make_donor_dataset_major_gift_flag(): assert (flagged["gift_amount"] >= 500.0).all() -def test_make_donor_dataset_from_utils_still_works_and_warns(): - with pytest.warns(DeprecationWarning, match="removed in 0.8.0"): - from philanthropy.utils import make_donor_dataset - - df = make_donor_dataset(n_donors=5, random_state=0) - assert df["donor_id"].nunique() == 5 - - -def test_make_donor_dataset_shim_emits_exactly_one_warning(): - import importlib +def test_make_donor_dataset_alias_removed_from_utils(): + # Deprecated in 0.7.0, removed in 0.8.0: the only import path is datasets. + import philanthropy.utils - with warnings.catch_warnings(record=True) as record: - warnings.simplefilter("always") - mod = importlib.import_module("philanthropy.utils") - mod.make_donor_dataset(n_donors=5, random_state=0) - dep = [w for w in record if issubclass(w.category, DeprecationWarning)] - assert len(dep) == 1 + assert "make_donor_dataset" not in philanthropy.utils.__all__ + with pytest.raises(ImportError): + from philanthropy.utils import make_donor_dataset # noqa: F401