Skip to content

Fix RFM NaN handling, drop dead numpy path, align lapse classes_ - #252

Merged
shivamlalakiya merged 3 commits into
mainfrom
f7b-rfm-lapse-forecast
Sep 26, 2026
Merged

shivamlalakiya merged 3 commits into
mainfrom
f7b-rfm-lapse-forecast

Conversation

@shivamlalakiya

Copy link
Copy Markdown
Contributor

Summary

Three small correctness fixes and one docs fix in preprocessing/models, found while reading through _rfm.py, _lapse.py, and _forecast.py:

  • RFMTransformer, behaviour change: a gift with a NaN gift_amount used to count toward frequency while being silently dropped from monetary, so the two columns described different sets of gifts. It now excludes that gift from both, with a UserWarning naming how many rows got dropped. This means frequency can come in lower than a donor's raw row count when some of their gifts have no recorded amount.
  • RFMTransformer, dead code: the docstring described a numpy-array input path (falling back to x0..xn column names), but _validate_input always required donor_id/gift_date/gift_amount by name, so that path always raised anyway and never actually worked. Removed the dead branches and now raise a clear TypeError up front telling the caller a DataFrame with named columns is required. Also added a line to the docstring noting the outputs are raw R/F/M values, not scores or frozen bins.
  • LapsePredictor: classes_ is now built with sklearn.utils.multiclass.unique_labels, matching every other classifier in the package (PropensityScorer, etc.) instead of a bare np.unique(y).
  • FinancialForecastModel: the module docstring's title called it a "Hybrid LSTM-ARIMA" forecaster without the caveat the class docstring carries further down. Added the same "it's really LinearRegression + MLPRegressor on residuals + a hand-rolled AR(p), no actual LSTM or ARIMA" note to the title.

CHANGELOG.md has a new ### Fixed entry under [Unreleased] covering all four.

Test plan

  • Added test_rfm_transformer_nan_amount_excluded_from_frequency_and_monetary in tests/test_rfm.py, confirmed it fails against the old code (frequency=2 instead of 1, no warning) and passes with the fix.
  • Updated test_rfm_transformer_validation and renamed/updated test_rfm_transformer_ndarray_input_raises_missing_columns to match the new TypeError message for non-DataFrame input.
  • Added tests/test_lapse.py (none existed before) with a basic fit/classes_ test and a lock-in test that classes_ matches unique_labels(y).
  • make ci: 2198 passed, 30 skipped, coverage 98.35% (floor 92%), flake8 and mypy clean.
  • make riskcov: risk-tier coverage 98% against the 93% floor.

… classes_

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. A gift with no amount is now excluded from both, with a
UserWarning naming how many rows were dropped; the docstring spells out that
frequency can now come in lower than the donor's raw row count.

RFMTransformer also documented a numpy-array input path that never actually
worked: fit() built x0..xn placeholder column names for an unnamed array,
but _validate_input always required donor_id/gift_date/gift_amount by name
and so always raised anyway. Removed the dead branches and replaced them
with a clear TypeError up front. Docstring now also notes the outputs are
raw R/F/M values, not scores or frozen bins.

LapsePredictor.classes_ is now built with sklearn.utils.multiclass's
unique_labels, matching the other classifiers instead of a bare np.unique.

FinancialForecastModel's module docstring now carries the same "no actual
LSTM or ARIMA, just LinearRegression + MLPRegressor + a hand-rolled AR(p)"
caveat in its title that the class docstring already had further down.
@shivamlalakiya
shivamlalakiya merged commit 862c438 into main Sep 26, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant