Skip to content

fix: remove dead branch in WealthPercentileTransformer.fit (#168) - #176

Closed
Larslllllll wants to merge 2 commits into
PhilanthroPy-Project:mainfrom
Larslllllll:fix/wealth-percentile-dead-branch
Closed

fix: remove dead branch in WealthPercentileTransformer.fit (#168)#176
Larslllllll wants to merge 2 commits into
PhilanthroPy-Project:mainfrom
Larslllllll:fix/wealth-percentile-dead-branch

Conversation

@Larslllllll

Copy link
Copy Markdown
Contributor

Summary

The hasattr(X, "columns") branch in fit() never executed because validate_data() returns a NumPy array, which never has a .columns attribute. validate_data() itself sets feature_names_in_ when given a DataFrame, so the column names are preserved regardless.

The elif branch (array-input path) was doing all the work. This PR deletes the dead branch and promotes the elif to a plain if with an explanatory comment.

Changes

  • Deleted the dead if hasattr(X, "columns"): branch
  • Promoted the elif to a plain if with comment explaining why the check is on self not X
  • Added test test_wealth_percentile_feature_names_match_for_frame_and_array_input to pin expected feature names for both DataFrame and array inputs

Verification

# Verify dead branch is gone
test "$(grep -c 'hasattr(X, "columns")' philanthropy/preprocessing/_wealth_percentile.py)" = "0"

# Verify behavior is preserved
python -c "
import numpy as np, pandas as pd
from philanthropy.preprocessing import WealthPercentileTransformer
df = pd.DataFrame({'net_worth': [1.0, 2.0, 3.0], 'other': [1.0, 2.0, 3.0]})
a = WealthPercentileTransformer().fit(df)
b = WealthPercentileTransformer().fit(df.to_numpy())
assert list(a.get_feature_names_out()) == ['net_worth', 'other', 'net_worth_pct_rank']
assert list(b.get_feature_names_out()) == ['x0', 'x1']
"

Fixes #168

Lars added 2 commits September 6, 2026 02:06
…umns

Add two unit tests pinning the behaviour of all-NaN and partially-NaN
wealth columns: the all-missing branch must return NaN ranks, keep a
stable output width, and raise no warning; the partial-missing branch
must rank only observed values and propagate NaN to the rank column.

Closes PhilanthroPy-Project#169.
The hasattr(X, "columns") branch in fit() never executed because
validate_data() returns a NumPy array, which never has a .columns
attribute. validate_data() itself sets feature_names_in_ when given
a DataFrame, so the column names are preserved regardless.

The elif branch (array-input path) was doing all the work. Deleted
the dead branch and promoted the elif to a plain if.

Added test_wealth_percentile_feature_names_match_for_frame_and_array_input
to pin the expected feature names for both input types, proving the
deletion is safe.

Fixes PhilanthroPy-Project#168
shivamlalakiya pushed a commit that referenced this pull request Sep 6, 2026
Closes #156. WealthPercentileTransformer.fit now raises an actionable ValueError when an explicit wealth_cols list matches no training column; partial matches and automatic detection are unchanged. Also removes the dead hasattr(X, 'columns') branch in fit, which closes #168 and subsumes PRs #175 and #176.
@shivamlalakiya

Copy link
Copy Markdown
Contributor

Thank you, @Larslllllll. Closing this one as already-landed rather than rejected, and your credit is safe on both counts.

Where each piece went:

So everything in this PR is on main, which is also why it now shows as conflicting: it is trying to re-add lines that are already there.

The duplication with #175 the same evening was our process failure, not yours. A "claim the issue before you open a PR" rule is going into CONTRIBUTING.md so contributors stop colliding like this.

Closing as already merged via #174 and #179.

shivamlalakiya added a commit that referenced this pull request Sep 6, 2026
…attempt (#187)

Two follow-ups after the #175/#176 duplicate-PR triage:

- CONTRIBUTING.md gets a "Claiming an issue" rule (comment before opening a
  PR, wait for maintainer assignment) so two people never again spend an
  evening fixing the same four-line bug. The issue-draft template states the
  same rule inline, so every newly filed good-first-issue carries it without
  depending on a reader following a link.

- @HeaTTap independently identified and fixed the same dead
  `hasattr(X, "columns")` branch as @Larslllllll, via #175. That PR was
  closed as subsumed by #179 (which carried the same fix), but @HeaTTap had
  no CONTRIBUTORS.md line as a result; Lars was already credited via #174.
  This adds it.
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.

cleanup: WealthPercentileTransformer.fit has a branch that can never run

2 participants