fix: WealthPercentileTransformer silent no-op and dead branch, EncounterTransformer datetime64 rejection - #186
Closed
shivamlalakiya wants to merge 1 commit into
Closed
Conversation
…terTransformer datetime64 rejection Three real defects, plus the process fix for how the last one happened: - WealthPercentileTransformer(wealth_cols=[...]) now raises ValueError instead of silently training without the wealth signal when none of the named columns exist in X. A typo'd column name was indistinguishable from an intentional no-op. Closes #156. - WealthPercentileTransformer.fit's dead `hasattr(X, "columns")` branch is removed (validate_data already returns a NumPy array and already sets feature_names_in_ for DataFrame input). Two contributors independently fixed this same branch the same evening, #175 (@HeaTTap) and #176 (@Larslllllll); this lands that fix and credits both in CONTRIBUTORS.md. Closes #168. - EncounterTransformer no longer rejects a gift_date column already parsed to datetime64. Any real loader (pd.read_csv(parse_dates=...), a SQL read, this package's own make_donor_panel) hands back exactly that column mix, and validate_data's numpy promotion of datetime64 against float64 raised before the transformer got a chance to read it. Closes #163. - CONTRIBUTING.md gets a "Claiming an issue" rule (comment before you open a PR, wait for assignment) so the #175/#176 duplication doesn't repeat; the issue-draft template now states it inline too. All three fixes are covered by new regression tests; full suite, make ci, and make riskcov all green.
Contributor
Author
|
Closing this in favor of already-open external contributor PRs for the same issues: #180 (@be-student, issue #163) and #179 (@be-student, issue #156, which also removes the #168 dead branch). Those landed first, are already green, and external contribution signal is a tracked JOSS blocker, so they should merge over a duplicate from me. The parts of this PR not covered by #179/#180 (the CONTRIBUTING.md "Claiming an issue" rule and the issue-draft template line) are being handled separately to avoid a third duplicate. |
shivamlalakiya
deleted the
fix/wealth-percentile-encounter-bugs-and-process
branch
September 6, 2026 19:05
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 real defects found during a pass over the JOSS-prep plan, plus the process fix for how the third one happened.
WealthPercentileTransformer(wealth_cols=[...])now raisesValueErrorinstead of silently training without the wealth signal when none of the named columns exist inX. A typo'd column name was indistinguishable from an intentional no-op. ClosesWealthPercentileTransformersilently ignores awealth_colslist that matches no column #156.WealthPercentileTransformer.fit's deadhasattr(X, "columns")branch is removed (validate_dataalready returns a NumPy array and already setsfeature_names_in_for DataFrame input). Two contributors independently fixed this same branch the same evening, fix: remove dead hasattr branch in WealthPercentileTransformer.fit (#168) #175 (@HeaTTap) and fix: remove dead branch in WealthPercentileTransformer.fit (#168) #176 (@Larslllllll); this PR lands that fix and credits both inCONTRIBUTORS.md. Closes cleanup:WealthPercentileTransformer.fithas a branch that can never run #168.EncounterTransformerno longer rejects agift_datecolumn already parsed todatetime64. Any real loader (pd.read_csv(parse_dates=...), a SQL read, this package's ownmake_donor_panel) hands back exactly that column mix, andvalidate_data's numpy promotion ofdatetime64againstfloat64raised before the transformer got a chance to read it. Closes EncounterTransformer rejects a parsed datetime64 gift_date and accepts only date strings #163.CONTRIBUTING.mdgets a "Claiming an issue" rule (comment before you open a PR, wait for assignment) so the fix: remove dead hasattr branch in WealthPercentileTransformer.fit (#168) #175/fix: remove dead branch in WealthPercentileTransformer.fit (#168) #176 duplication doesn't repeat; the issue-draft template states it inline too.Also filed 5 good-first-issue drafts from
scripts/issue-drafts/as real GitHub issues (#181-#185), each re-verified against current line numbers/greps first. Not part of this diff since they're process, not code.Test plan
test_unmatched_wealth_cols_raises,test_feature_names_match_for_frame_and_array_input,test_accepts_parsed_datetime64_gift_datemake cigreenmake riskcovgreen (97% branch over the risk-tier subtree, 93% floor)