Skip to content

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

Closed
HeaTTap wants to merge 2 commits into
PhilanthroPy-Project:mainfrom
HeaTTap:cleanup/wealth-percentile-fit
Closed

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

Conversation

@HeaTTap

@HeaTTap HeaTTap commented Sep 6, 2026

Copy link
Copy Markdown

What & why

Resolves #168

In WealthPercentileTransformer.fit, validate_data returns a NumPy array so hasattr(X, "columns") was never True and that branch never executed. The elif branch beneath it was handling both DataFrame and ndarray inputs.

This PR:

  1. Deletes the dead hasattr(X, "columns") branch in WealthPercentileTransformer.fit and promotes the elif condition to a regular if statement with an explanatory comment.
  2. Adds unit test test_wealth_percentile_feature_names_match_for_frame_and_array_input in tests/test_preprocessing.py verifying that get_feature_names_out preserves column names for DataFrame inputs and generates x0..xn for ndarray inputs.
  3. Adds entry to CHANGELOG.md and CONTRIBUTORS.md.

Checklist

  • make ci passes locally (lint -> collection -> tests -> coverage >= 92%)
  • New/changed public API has docstrings and is exported in the subpackage __init__.py
  • Tests added or updated
  • CHANGELOG.md updated under [Unreleased]
  • Added yourself to CONTRIBUTORS.md (skip if you would rather not be listed)

@shivamlalakiya

Copy link
Copy Markdown
Contributor

Thanks for this, the fix matches #168 exactly. There's a merge conflict now against main (PR #174, which pins the same class's edge-case tests, merged first), so this needs a rebase before it can go in. Both PRs happened to touch the same three spots, and the fix in each case is to keep both sides, not pick one:

tests/test_preprocessing.py

  • The WealthPercentileTransformer import ends up listed twice — delete one.
  • Both PRs add a test right after test_crm_cleaner_on_unnamed_ndarrays_skips_named_columns: test: add edge-case tests for WealthPercentileTransformer missing columns (#169) #174 adds a class TestWealthPercentileTransformer with two tests, this PR adds a standalone test_wealth_percentile_feature_names_match_for_frame_and_array_input. Keep both; I'd suggest moving your standalone test into that class (as a third method, dropping the wealth_percentile_ prefix since it's now scoped) so all three live together, but a standalone function next to the class works too.

CONTRIBUTORS.md

CHANGELOG.md

  • Same as above, auto-merges cleanly, just confirm both entries are present after rebase.
git fetch origin
git rebase origin/main
# resolve the three spots above, then:
git add -A && git rebase --continue
git push --force-with-lease

Once that's green I'll merge it. Nice first PR, thanks for tightening this up.

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 for this, @HeaTTap, and sorry for the outcome: the fix is correct but it has landed by another route.

Issue #168 is now closed by #179, which was merged as 6dfafa5. That PR removed the same dead hasattr(X, "columns") branch as part of a larger change to WealthPercentileTransformer.fit for #156, so the one-line deletion here is already on main. Your diagnosis was right, and the get_feature_names_out assertion you added is equivalent to the coverage that #174 and #179 landed.

Two contributors independently opened a fix for #168 the same evening (#175 and #176), which is our process failure, not yours. We are adding a "claim the issue before you open a PR" rule to CONTRIBUTING.md so this stops happening.

You are owed a CONTRIBUTORS.md line and do not have one yet. That is tracked and will be added in a follow-up PR, not dropped.

Closing as subsumed by #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.
@shivamlalakiya

Copy link
Copy Markdown
Contributor

@HeaTTap following up on this one properly rather than leaving it as a silent close.

You and another contributor solved the same four-line problem on the same evening, and closing yours cost you that evening for nothing. That was the project's process failure, not yours. There was no way to claim an issue before opening a pull request, so there was no way for either of you to know the other was working on it. The only person who could see both was me, and I didn't act on it fast enough.

That's fixed now. #187 is merged and adds a claim-before-PR rule to CONTRIBUTING.md, so an issue gets assigned before anyone writes code, and it credits you by name in CHANGELOG.md for the fix attempt.

If you want another one, the good first issue label has open ones and I'll assign whichever you comment on within 24 hours. And if the setup or the contribution process was annoying in any other way, I'd genuinely like to hear it: #188

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