diff --git a/CHANGELOG.md b/CHANGELOG.md index a8bbb40..c5a4273 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,28 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) failed to join against the same donor's `"123"` from a column that never had a blank. Integral floats are now normalised to their bare digits first. +### Fixed +- `RFMTransformer` counted a gift with a NaN `gift_amount` toward `frequency` + while silently dropping it from `monetary`, so the two columns described + different sets of gifts. **Behaviour change:** a gift with no amount is now + excluded from `frequency` as well as from `monetary`, with a `UserWarning` + naming how many rows were dropped, so a donor's gift count can drop if some + of their gifts have no recorded amount. +- `RFMTransformer.fit`/`transform` no longer pretend to accept a bare numpy + array: the documented `x0..xn` column-naming path was dead code, since + `_validate_input` always required `donor_id`/`gift_date`/`gift_amount` by + name and so always raised on array input anyway. A numpy array now gets a + clear `TypeError` up front instead of failing deeper in `_cut`/ + `_validate_input`. The docstring also now says outputs are raw R/F/M + values, not scores or frozen bins. +- `LapsePredictor.classes_` is now built with `sklearn.utils.multiclass. + unique_labels`, matching the other classifiers, instead of a bare + `np.unique(y)`. +- `FinancialForecastModel`'s module docstring now names its actual backend + (`LinearRegression` + `MLPRegressor` on the residuals + a hand-rolled + AR(p) roll-forward) up front, alongside the "Hybrid LSTM-ARIMA" title, + instead of only in the class docstring further down. + ## [0.8.0] - 2026-09-24 The first release with a Raiser's Edge on-ramp and `as_of` scoring cutoffs on the diff --git a/philanthropy/models/_forecast.py b/philanthropy/models/_forecast.py index 74a5192..3e03357 100644 --- a/philanthropy/models/_forecast.py +++ b/philanthropy/models/_forecast.py @@ -1,7 +1,9 @@ """ philanthropy.models._forecast ============================== -Hybrid LSTM-ARIMA revenue/giving forecaster for nonprofit advancement teams. +Hybrid LSTM-ARIMA revenue/giving forecaster for nonprofit advancement teams +(in practice: LinearRegression plus an MLPRegressor on the residuals, plus a +hand-rolled AR(p) roll-forward; no actual LSTM or ARIMA implementation). Nonprofit and academic medical centre (AMC) advancement shops plan campaigns, staffing, and cash flow around *forward* estimates of giving revenue. The diff --git a/philanthropy/models/_lapse.py b/philanthropy/models/_lapse.py index 42d27d2..fac12aa 100755 --- a/philanthropy/models/_lapse.py +++ b/philanthropy/models/_lapse.py @@ -13,6 +13,7 @@ from sklearn.ensemble import RandomForestClassifier from sklearn.utils.validation import check_is_fitted, validate_data from sklearn.utils import Tags +from sklearn.utils.multiclass import unique_labels _Self = TypeVar("_Self", bound="LapsePredictor") @@ -67,7 +68,7 @@ def fit(self: _Self, X: Any, y: Any) -> _Self: self : LapsePredictor """ X, y = validate_data(self, X, y, ensure_all_finite="allow-nan", reset=True) - self.classes_ = np.unique(y) + self.classes_ = unique_labels(y) self.n_features_in_ = X.shape[1] self.estimator_ = RandomForestClassifier( diff --git a/philanthropy/preprocessing/_rfm.py b/philanthropy/preprocessing/_rfm.py index 3acd062..08d44fb 100755 --- a/philanthropy/preprocessing/_rfm.py +++ b/philanthropy/preprocessing/_rfm.py @@ -52,6 +52,23 @@ class RFMTransformer(TransformerMixin, BaseEstimator): [Fader, Hardie and Lee 2005]; ``tenure`` is that T. Defaults to False so the output shape does not change under existing callers, and will become the default in the next major release. + + Notes + ----- + Output ``recency``, ``frequency``, and ``monetary`` are raw counts and + sums, not scores or frozen bins: callers who want quintile-style RFM + scores bin these columns themselves downstream. + + A gift with a NaN ``gift_amount`` is excluded from **both** + ``frequency`` and ``monetary``, not just ``monetary``: an unknown amount + means the gift's contribution to either column is unknown, so it is + dropped from the count as well as the sum, with a ``UserWarning`` naming + how many rows were dropped. This means ``frequency`` can be lower than + the donor's raw row count in the input. + + ``X`` must be a ``pandas.DataFrame`` with named columns; a bare numpy + array has no ``donor_id`` / ``gift_date`` / ``gift_amount`` to key off of + and is rejected in :meth:`fit` with a ``TypeError``. """ def __init__( self, @@ -69,9 +86,10 @@ def fit(self: _Self, X: Any, y: Any = None) -> _Self: """Fit the transformer by validating input and freezing the reference date. Parameters ---------- - X : array-like of shape (n_samples, n_features) + X : pd.DataFrame of shape (n_samples, n_features) Transaction log with required columns ``donor_id``, ``gift_date``, and - ``gift_amount``. + ``gift_amount``. Must be a ``pandas.DataFrame`` with those columns + named; a bare numpy array has no way to name them and is rejected. y : ignored Present for scikit-learn API compatibility. Returns @@ -92,19 +110,21 @@ def fit(self: _Self, X: Any, y: Any = None) -> _Self: batches. Raises ------ + TypeError + If ``X`` is not a pandas DataFrame. ValueError If ``X`` is missing any of the required columns ``donor_id``, ``gift_date``, or ``gift_amount``. """ - # Manual validation to avoid name/length strictness during fit - if hasattr(X, "columns"): - self.feature_names_in_ = np.array(X.columns.tolist(), dtype=object) - self.n_features_in_ = len(self.feature_names_in_) - else: - X_arr = np.asarray(X) - self.n_features_in_ = X_arr.shape[1] - self.feature_names_in_ = np.array([f"x{i}" for i in range(self.n_features_in_)], dtype=object) - + if not hasattr(X, "columns"): + raise TypeError( + "RFMTransformer requires a pandas DataFrame with named " + "columns (donor_id, gift_date, gift_amount); a numpy array " + "has no column names to validate against." + ) + self.feature_names_in_ = np.array(X.columns.tolist(), dtype=object) + self.n_features_in_ = len(self.feature_names_in_) + self._validate_input(X) # Freeze the recency reference date from TRAINING data (leakage-safety @@ -126,24 +146,30 @@ def transform(self, X: Any) -> pd.DataFrame: Parameters ---------- - X : array-like of shape (n_samples, n_features) + X : pd.DataFrame of shape (n_samples, n_features) Transaction log with required columns ``donor_id``, ``gift_date``, and - ``gift_amount``. Rows are gift-level; output is donor-level. + ``gift_amount``. Rows are gift-level; output is donor-level. Must be + a ``pandas.DataFrame`` with those columns named; a bare numpy array + is rejected. Returns ------- rfm_df : pd.DataFrame of shape (n_donors, 4 or 5) - Donor-level RFM features: + Donor-level RFM features, as raw values (not scores or bins): * ``donor_id`` : object Unique donor identifier. * ``recency`` : int64 Days since each donor's most recent gift, relative to the frozen ``reference_date_``. * ``frequency`` : int64 - Total number of gifts per donor in the (possibly ``as_of``-filtered) - transaction log. + Total number of gifts per donor with a known ``gift_amount``, in + the (possibly ``as_of``-filtered) transaction log. Gifts with a + NaN ``gift_amount`` are excluded from both ``frequency`` and + ``monetary``, with a warning, so the two counts describe the + same set of gifts. * ``monetary`` : float64 Aggregated gift amount per donor (sum, mean, or other function - specified by ``agg_func``). + specified by ``agg_func``), over gifts with a known + ``gift_amount``. * ``tenure`` : int64 *Optional, present only if ``include_tenure=True``.* Days from each donor's first gift to the frozen ``reference_date_``. @@ -158,11 +184,15 @@ def transform(self, X: Any) -> pd.DataFrame: ``gift_date``, or ``gift_amount``. """ check_is_fitted(self) - if not hasattr(X, "columns") and not isinstance(X, pd.DataFrame): - raise TypeError("X must be a pandas DataFrame") + if not hasattr(X, "columns"): + raise TypeError( + "RFMTransformer requires a pandas DataFrame with named " + "columns (donor_id, gift_date, gift_amount); a numpy array " + "has no column names to validate against." + ) # Manual validation self._validate_input(X) - + X_df = self._cut(X) self._warn_if_unbounded(X_df) @@ -171,17 +201,38 @@ def transform(self, X: Any) -> pd.DataFrame: ref_date = self.reference_date_ grouped = X_df.groupby('donor_id') - + # Recency: Days since the last gift relative to reference_date last_gift = grouped['gift_date'].max() recency = (ref_date - last_gift).dt.days - - # Frequency: Total number of gifts - frequency = grouped['gift_date'].count() - - # Monetary: Average or cumulative gift amount depending on agg_func - monetary = grouped['gift_amount'].agg(self.agg_func) - + + # Frequency and monetary must describe the same gifts: a gift with an + # unknown amount doesn't count toward either, so it isn't silently + # counted in frequency while being dropped from monetary. + has_amount = X_df['gift_amount'].notna() + n_missing_amount = int((~has_amount).sum()) + if n_missing_amount: + warnings.warn( + f"RFMTransformer is excluding {n_missing_amount} gift row(s) " + "with a NaN gift_amount from both frequency and monetary, so " + "the two features describe the same set of gifts.", + UserWarning, + stacklevel=2, + ) + grouped_valid = X_df[has_amount].groupby('donor_id') + + # Frequency: number of gifts with a known amount + frequency = grouped_valid['gift_date'].count().reindex( + recency.index, fill_value=0 + ) + + # Monetary: aggregated gift amount depending on agg_func, over gifts + # with a known amount. A donor with no such gifts has no defined + # aggregate, so it stays NaN rather than being coerced to 0. + monetary = grouped_valid['gift_amount'].agg(self.agg_func).reindex( + recency.index + ) + rfm_df = pd.DataFrame({ 'donor_id': recency.index, 'recency': recency.values, @@ -200,9 +251,7 @@ def transform(self, X: Any) -> pd.DataFrame: def _cut(self, X: Any) -> pd.DataFrame: """Copy ``X``, parse ``gift_date``, and drop gifts after ``as_of``.""" - X_df = X.copy() if hasattr(X, "columns") else pd.DataFrame( - X, columns=self.feature_names_in_ - ) + X_df = X.copy() X_df['gift_date'] = pd.to_datetime(X_df['gift_date']) return _apply_as_of_cutoff( X_df, 'gift_date', self.as_of, "RFMTransformer", row_noun="gift" @@ -233,9 +282,8 @@ def _warn_if_unbounded(self, X_df: pd.DataFrame) -> None: ) def _validate_input(self, X: Any) -> None: - cols = X.columns if hasattr(X, "columns") else self.feature_names_in_ required_cols = {"donor_id", "gift_date", "gift_amount"} - if not required_cols.issubset(cols): + if not required_cols.issubset(X.columns): raise ValueError(f"X must contain columns: {required_cols}") def get_feature_names_out(self, input_features: Any = None) -> np.ndarray: diff --git a/tests/test_lapse.py b/tests/test_lapse.py new file mode 100644 index 0000000..e175193 --- /dev/null +++ b/tests/test_lapse.py @@ -0,0 +1,33 @@ +import numpy as np + +from philanthropy.models import LapsePredictor + + +def _dummy_X(n): + rng = np.random.RandomState(0) + return rng.rand(n, 3) + + +def test_lapse_predictor_binary_classes(): + X = _dummy_X(20) + y = np.array([0, 1] * 10) + clf = LapsePredictor(n_estimators=5, random_state=0) + clf.fit(X, y) + assert list(clf.classes_) == [0, 1] + + +def test_lapse_predictor_classes_built_with_unique_labels(): + # LapsePredictor.classes_ is now built with sklearn.utils.multiclass's + # unique_labels, matching PropensityScorer and the other sibling + # classifiers, instead of a bare np.unique(y). For the plain integer + # binary target this classifier is documented for, the two calls agree + # on the result; this test locks in that the fitted classes_ still comes + # out sorted and matches unique_labels' own output exactly, so a future + # edit can't silently drift the two apart again. + from sklearn.utils.multiclass import unique_labels + + X = _dummy_X(20) + y = np.array([1, 0] * 10) + clf = LapsePredictor(n_estimators=5, random_state=0) + clf.fit(X, y) + assert np.array_equal(clf.classes_, unique_labels(y)) diff --git a/tests/test_rfm.py b/tests/test_rfm.py index 8989e12..7a214f8 100755 --- a/tests/test_rfm.py +++ b/tests/test_rfm.py @@ -73,10 +73,32 @@ def test_rfm_transformer_validation(): with pytest.raises(ValueError, match="X must contain columns:"): transformer.fit(data) - with pytest.raises(TypeError, match="X must be a pandas DataFrame"): + with pytest.raises(TypeError, match="requires a pandas DataFrame"): transformer.transform([1, 2, 3]) -def test_rfm_transformer_ndarray_input_raises_missing_columns(): - with pytest.raises(ValueError, match="donor_id"): +def test_rfm_transformer_ndarray_input_rejected(): + # numpy input has no donor_id / gift_date / gift_amount to key off of; + # RFMTransformer requires a DataFrame with named columns instead of + # silently falling back to x0..xn placeholder names. + with pytest.raises(TypeError, match="requires a pandas DataFrame"): RFMTransformer().fit(np.ones((3, 3))) + + +def test_rfm_transformer_nan_amount_excluded_from_frequency_and_monetary(): + # A gift with an unknown amount should not count toward frequency while + # being silently dropped from monetary: the two features must describe + # the same set of gifts. + data = pd.DataFrame({ + 'donor_id': [1, 1], + 'gift_date': ['2023-01-01', '2023-06-01'], + 'gift_amount': [100, np.nan], + }) + + transformer = RFMTransformer() + with pytest.warns(UserWarning, match="excluding 1 gift row"): + rfm = transformer.fit_transform(data) + + d1 = rfm[rfm['donor_id'] == 1].iloc[0] + assert d1['frequency'] == 1 + assert d1['monetary'] == 100