From f4d663b50be615fc7a874e94cd2c41d1cf902f2b Mon Sep 17 00:00:00 2001 From: shubhrai23 Date: Wed, 26 Aug 2026 14:25:20 +0530 Subject: [PATCH] DEP: warn when drop_repeat_donors default is used FiscalYearGroupedSplitter(drop_repeat_donors=...) now defaults to 'warn', which emits a DeprecationWarning telling the caller that the default will change from False to True in 0.8.0. Internally the splitter still treats 'warn' as False, so behaviour is unchanged. - Add deprecation entry to DEPRECATIONS registry in test_deprecations.py - Update all existing tests to pass drop_repeat_donors=False explicitly or wrap in pytest.warns(DeprecationWarning) - Silence doctests by passing drop_repeat_donors=False - Update CHANGELOG.md under [Unreleased] / Deprecated - Fix test_leakage.py to pass drop_repeat_donors=False explicitly Closes #108 --- CHANGELOG.md | 3 + .../_temporal_donor_splitter.py | 30 +++++++-- tests/test_deprecations.py | 5 ++ tests/test_leakage.py | 2 +- tests/test_model_selection.py | 64 ++++++++++++------- 5 files changed, 74 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a415057..b20ac2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## [Unreleased] +### Deprecated +- `FiscalYearGroupedSplitter`'s default for `drop_repeat_donors` (currently `False`) is deprecated and will change to `True` in 0.8.0. Leaving it at its default now emits a `DeprecationWarning`. Pass `drop_repeat_donors=False` explicitly to silence the warning and retain current behavior. Closes #108, by @shubhrai23. + ### Added - `credit-guard` CI job: pull requests touching `philanthropy/` must also update this changelog, and the author must be credited in diff --git a/philanthropy/model_selection/_temporal_donor_splitter.py b/philanthropy/model_selection/_temporal_donor_splitter.py index d1f9ad0..08517e4 100755 --- a/philanthropy/model_selection/_temporal_donor_splitter.py +++ b/philanthropy/model_selection/_temporal_donor_splitter.py @@ -24,7 +24,7 @@ >>> from philanthropy.model_selection import FiscalYearGroupedSplitter >>> X = np.zeros((100, 3)) >>> fiscal_years = np.array([2019]*20 + [2020]*30 + [2021]*25 + [2022]*25) ->>> splitter = FiscalYearGroupedSplitter(n_splits=3) +>>> splitter = FiscalYearGroupedSplitter(n_splits=3, drop_repeat_donors=False) >>> splits = list(splitter.split(X, groups=fiscal_years)) >>> len(splits) 3 @@ -72,6 +72,12 @@ class FiscalYearGroupedSplitter(BaseCrossValidator): 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. + Whether to remove from each test fold any donor who already appears in that fold's training rows. @@ -115,7 +121,7 @@ class FiscalYearGroupedSplitter(BaseCrossValidator): >>> from philanthropy.model_selection import FiscalYearGroupedSplitter >>> X = np.zeros((200, 5)) >>> fy = np.array([2018]*40 + [2019]*50 + [2020]*55 + [2021]*30 + [2022]*25) - >>> splitter = FiscalYearGroupedSplitter(n_splits=3, gap_years=0) + >>> splitter = FiscalYearGroupedSplitter(n_splits=3, gap_years=0, drop_repeat_donors=False) >>> for train_idx, test_idx in splitter.split(X, groups=fy): ... train_fy = np.unique(fy[train_idx]) ... test_fy = np.unique(fy[test_idx]) @@ -178,12 +184,22 @@ def __init__( self, n_splits: int = 5, gap_years: int = 0, - drop_repeat_donors: bool = False, + drop_repeat_donors: bool | str = "warn", ) -> 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 @@ -251,6 +267,7 @@ def split(self, X, y=None, groups=None): 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) if groups is None: raise ValueError( @@ -261,7 +278,7 @@ def split(self, X, y=None, groups=None): groups_arr = np.asarray(groups) donor_ids = None - if self.drop_repeat_donors: + if drop_repeat: if groups_arr.ndim != 2 or groups_arr.shape[1] != 2: raise ValueError( "drop_repeat_donors=True requires `groups` with shape " @@ -385,7 +402,8 @@ def get_n_splits(self, X=None, y=None, groups=None) -> int: n_splits, gap_years = self._validate_params() if groups is not None: groups = np.asarray(groups) - if self.drop_repeat_donors and groups.ndim == 2 and groups.shape[1] == 2: + drop_repeat = False if self.drop_repeat_donors == "warn" else 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) n_fy = len(unique_fy) @@ -402,7 +420,7 @@ def __repr__(self) -> str: f"{self.__class__.__name__}(" f"n_splits={self.n_splits}, " f"gap_years={self.gap_years}, " - f"drop_repeat_donors={self.drop_repeat_donors})" + f"drop_repeat_donors={self.drop_repeat_donors!r})" ) diff --git a/tests/test_deprecations.py b/tests/test_deprecations.py index ddee746..167701a 100644 --- a/tests/test_deprecations.py +++ b/tests/test_deprecations.py @@ -43,6 +43,11 @@ # 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(), + ), ] diff --git a/tests/test_leakage.py b/tests/test_leakage.py index 54a26a2..839940d 100755 --- a/tests/test_leakage.py +++ b/tests/test_leakage.py @@ -622,7 +622,7 @@ def test_row_split_flatters_conformal_coverage_and_a_donor_split_does_not(): # Flag off: the FY2021 fold keeps donors that are also in the FY2020 # calibration rows. Flag on: those donors are dropped. - splitter = FiscalYearGroupedSplitter(n_splits=1) + splitter = FiscalYearGroupedSplitter(n_splits=1, drop_repeat_donors=False) cal_idx, all_test_idx = next( iter(splitter.split(X_later, groups=fy_later)) ) diff --git a/tests/test_model_selection.py b/tests/test_model_selection.py index 0fb5092..cbf2d05 100644 --- a/tests/test_model_selection.py +++ b/tests/test_model_selection.py @@ -9,32 +9,32 @@ def test_groups_none_raises(): - s = FiscalYearGroupedSplitter(n_splits=2) + s = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=2) with pytest.raises(ValueError, match="groups"): list(s.split(np.zeros((10, 2)))) def test_length_mismatch_raises(): - s = FiscalYearGroupedSplitter(n_splits=2) + s = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=2) with pytest.raises(ValueError, match="match"): list(s.split(np.zeros((10, 2)), groups=[2020, 2021, 2022])) def test_single_fiscal_year_raises(): - s = FiscalYearGroupedSplitter(n_splits=2) + s = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=2) with pytest.raises(ValueError, match="at least 2"): list(s.split(np.zeros((10, 2)), groups=[2020] * 10)) def test_get_n_splits_without_groups(): - assert FiscalYearGroupedSplitter(n_splits=4).get_n_splits() == 4 + assert FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=4).get_n_splits() == 4 def test_cross_val_score_integration(): fy = np.array([2018] * 40 + [2019] * 50 + [2020] * 55 + [2021] * 30 + [2022] * 25) X = np.zeros((len(fy), 3)) y = np.random.default_rng(0).integers(0, 2, len(fy)) - s = FiscalYearGroupedSplitter(n_splits=3) + s = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=3) scores = cross_val_score( DummyClassifier(strategy="most_frequent"), X, y, cv=s, groups=fy ) @@ -42,14 +42,14 @@ def test_cross_val_score_integration(): def test_n_samples_list_input(): - s = FiscalYearGroupedSplitter(n_splits=2) + s = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=2) X = [[0, 0]] * 10 # plain list, no .shape attribute groups = [2019] * 3 + [2020] * 3 + [2021] * 4 assert len(list(s.split(X, groups=groups))) == 2 def test_n_samples_none_raises(): - s = FiscalYearGroupedSplitter() + s = FiscalYearGroupedSplitter(drop_repeat_donors=False) with pytest.raises(ValueError): list(s.split(None, groups=[2019, 2020])) @@ -62,7 +62,7 @@ def test_n_samples_none_raises(): def test_gap_years_withholds_the_year_before_each_test_fold(): X = np.zeros((10, 2)) - splitter = FiscalYearGroupedSplitter(n_splits=2, gap_years=1) + splitter = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=2, gap_years=1) splits = list(splitter.split(X, groups=_FY_GROUPS)) assert len(splits) == 2 @@ -78,7 +78,7 @@ def test_default_splitter_no_leakage_gap_years_zero(): """Default gap_years=0: training fold never contains a FY at or after the test FY, and each test fold is exactly one fiscal year.""" X = np.zeros((10, 2)) - splitter = FiscalYearGroupedSplitter(n_splits=3) + splitter = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=3) splits = list(splitter.split(X, groups=_FY_GROUPS)) assert len(splits) == 3 @@ -103,16 +103,17 @@ def test_not_enough_fiscal_years_names_the_shortfall(): match=r"Not enough fiscal years \(5\) for n_splits=2 with gap_years=5\.\s+" r"Need at least 8 distinct fiscal years\.", ): - list(FiscalYearGroupedSplitter(n_splits=2, gap_years=5).split( + list(FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=2, gap_years=5).split( X, groups=_FY_GROUPS )) def test_repr_and_get_n_splits_reflect_the_groups(): - splitter = FiscalYearGroupedSplitter(n_splits=2, gap_years=1) + with pytest.warns(DeprecationWarning): + splitter = FiscalYearGroupedSplitter(n_splits=2, gap_years=1) assert repr(splitter) == ( "FiscalYearGroupedSplitter(n_splits=2, gap_years=1, " - "drop_repeat_donors=False)" + "drop_repeat_donors='warn')" ) assert splitter.get_n_splits(groups=_FY_GROUPS) == 2 @@ -129,7 +130,7 @@ def test_non_positive_n_splits_raises_instead_of_slicing(bad_n_splits): # get_n_splits(), so the disagreement is a real failure. X = np.zeros((100, 3)) fy = np.array([2019] * 25 + [2020] * 25 + [2021] * 25 + [2022] * 25) - splitter = FiscalYearGroupedSplitter(n_splits=bad_n_splits) + splitter = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=bad_n_splits) with pytest.raises(ValueError, match="n_splits must be >= 1"): list(splitter.split(X, groups=fy)) @@ -141,14 +142,14 @@ def test_negative_gap_years_raises(): X = np.zeros((100, 3)) fy = np.array([2019] * 25 + [2020] * 25 + [2021] * 25 + [2022] * 25) with pytest.raises(ValueError, match="gap_years must be >= 0"): - list(FiscalYearGroupedSplitter(gap_years=-1).split(X, groups=fy)) + list(FiscalYearGroupedSplitter(drop_repeat_donors=False, gap_years=-1).split(X, groups=fy)) def test_non_integer_params_raise(): X = np.zeros((100, 3)) fy = np.array([2019] * 50 + [2020] * 50) with pytest.raises(ValueError, match="must be integers"): - list(FiscalYearGroupedSplitter(n_splits="three").split(X, groups=fy)) + list(FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits="three").split(X, groups=fy)) @pytest.mark.parametrize("n_splits", [1, 2, 3]) @@ -158,7 +159,7 @@ def test_get_n_splits_matches_the_folds_actually_yielded(n_splits, gap_years): # that the two independent code paths stay in step. X = np.zeros((150, 3)) fy = np.array([2018] * 30 + [2019] * 30 + [2020] * 30 + [2021] * 30 + [2022] * 30) - splitter = FiscalYearGroupedSplitter(n_splits=n_splits, gap_years=gap_years) + splitter = FiscalYearGroupedSplitter(drop_repeat_donors=False, n_splits=n_splits, gap_years=gap_years) assert splitter.get_n_splits(groups=fy) == len(list(splitter.split(X, groups=fy))) @@ -185,8 +186,9 @@ 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. X, fy, donor = _repeat_donor_panel() - for train, test in FiscalYearGroupedSplitter(n_splits=2).split(X, groups=fy): - assert set(donor[train]) & set(donor[test]) == {1, 2, 3} + 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} def test_drop_repeat_donors_removes_the_overlap(): @@ -229,22 +231,38 @@ 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(): + 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) + + def test_drop_repeat_donors_is_off_by_default(): # BaseCrossValidator, not BaseEstimator, so there is no get_params here. - assert FiscalYearGroupedSplitter().drop_repeat_donors is False + with pytest.warns(DeprecationWarning): + assert FiscalYearGroupedSplitter().drop_repeat_donors == "warn" 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. - assert "drop_repeat_donors=False" in repr(FiscalYearGroupedSplitter()) + 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 repr(FiscalYearGroupedSplitter()) != repr( - FiscalYearGroupedSplitter(drop_repeat_donors=True) - ) + with pytest.warns(DeprecationWarning): + assert repr(FiscalYearGroupedSplitter()) != repr( + FiscalYearGroupedSplitter(drop_repeat_donors=True) + ) def test_missing_donor_id_is_treated_as_already_seen():