Fix RFM NaN handling, drop dead numpy path, align lapse classes_ - #252
Merged
Merged
Conversation
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small correctness fixes and one docs fix in preprocessing/models, found while reading through
_rfm.py,_lapse.py, and_forecast.py:gift_amountused to count towardfrequencywhile being silently dropped frommonetary, so the two columns described different sets of gifts. It now excludes that gift from both, with aUserWarningnaming how many rows got dropped. This meansfrequencycan come in lower than a donor's raw row count when some of their gifts have no recorded amount.x0..xncolumn names), but_validate_inputalways requireddonor_id/gift_date/gift_amountby name, so that path always raised anyway and never actually worked. Removed the dead branches and now raise a clearTypeErrorup 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.classes_is now built withsklearn.utils.multiclass.unique_labels, matching every other classifier in the package (PropensityScorer, etc.) instead of a barenp.unique(y).CHANGELOG.md has a new
### Fixedentry under[Unreleased]covering all four.Test plan
test_rfm_transformer_nan_amount_excluded_from_frequency_and_monetaryintests/test_rfm.py, confirmed it fails against the old code (frequency=2 instead of 1, no warning) and passes with the fix.test_rfm_transformer_validationand renamed/updatedtest_rfm_transformer_ndarray_input_raises_missing_columnsto match the newTypeErrormessage for non-DataFrame input.tests/test_lapse.py(none existed before) with a basic fit/classes_ test and a lock-in test thatclasses_matchesunique_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.