What's wrong
FiscalYearGroupedSplitter(drop_repeat_donors=...) defaults to False. For a
static per-donor label (e.g. is_major_donor, the label used in the README,
the benchmarks page, and scripts/benchmark_models.py), leaving it at the
default lets the same donor's rows land in both the train and test fold of a
split. That is the exact leakage this splitter exists to prevent, and it is
live in the default path, not an opt-in footgun.
Where
| File |
Locate with |
philanthropy/model_selection/_temporal_donor_splitter.py |
grep -n "drop_repeat_donors: bool = False" philanthropy/model_selection/_temporal_donor_splitter.py |
What to change
- Add a
DeprecationWarning in FiscalYearGroupedSplitter.split (or
__init__) when drop_repeat_donors is left at its default False,
stating that the default will become True in a future release and that
callers relying on repeat donors appearing in both folds should pass
drop_repeat_donors=False explicitly to silence it.
- Update the class docstring's
drop_repeat_donors parameter entry to state
the planned default flip and the release it lands in, following the same
pattern already used for WealthScreeningImputerKNN(group_col_idx=...)
(see CHANGELOG.md, ## [Unreleased] section, Deprecated).
- Add a
## [Unreleased] CHANGELOG.md entry and a CONTRIBUTORS.md line
in the same PR (required by AGENTS.md).
Tests to add or extend
- File:
tests/test_temporal_donor_splitter.py
- Add:
test_default_drop_repeat_donors_emits_deprecation_warning
- Add:
test_explicit_drop_repeat_donors_false_silences_warning
Done when
python -c "
import warnings
from philanthropy.model_selection import FiscalYearGroupedSplitter
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
FiscalYearGroupedSplitter()
assert any(issubclass(x.category, DeprecationWarning) for x in w)
"
make ci
First time here?
Read CONTRIBUTING.md and AGENTS.md.
Add a ## [Unreleased] CHANGELOG entry and yourself to CONTRIBUTORS.md in
the same PR.
What's wrong
FiscalYearGroupedSplitter(drop_repeat_donors=...)defaults toFalse. For astatic per-donor label (e.g.
is_major_donor, the label used in the README,the benchmarks page, and
scripts/benchmark_models.py), leaving it at thedefault lets the same donor's rows land in both the train and test fold of a
split. That is the exact leakage this splitter exists to prevent, and it is
live in the default path, not an opt-in footgun.
Where
philanthropy/model_selection/_temporal_donor_splitter.pygrep -n "drop_repeat_donors: bool = False" philanthropy/model_selection/_temporal_donor_splitter.pyWhat to change
DeprecationWarninginFiscalYearGroupedSplitter.split(or__init__) whendrop_repeat_donorsis left at its defaultFalse,stating that the default will become
Truein a future release and thatcallers relying on repeat donors appearing in both folds should pass
drop_repeat_donors=Falseexplicitly to silence it.drop_repeat_donorsparameter entry to statethe planned default flip and the release it lands in, following the same
pattern already used for
WealthScreeningImputerKNN(group_col_idx=...)(see
CHANGELOG.md,## [Unreleased]section,Deprecated).## [Unreleased]CHANGELOG.mdentry and aCONTRIBUTORS.mdlinein the same PR (required by
AGENTS.md).Tests to add or extend
tests/test_temporal_donor_splitter.pytest_default_drop_repeat_donors_emits_deprecation_warningtest_explicit_drop_repeat_donors_false_silences_warningDone when
First time here?
Read CONTRIBUTING.md and AGENTS.md.
Add a
## [Unreleased]CHANGELOG entry and yourself toCONTRIBUTORS.mdinthe same PR.