Skip to content

test: add edge-case tests for WealthPercentileTransformer missing columns (#169) - #174

Merged
shivamlalakiya merged 2 commits into
PhilanthroPy-Project:mainfrom
Larslllllll:test/wealth-percentile-missing-column
Sep 6, 2026
Merged

test: add edge-case tests for WealthPercentileTransformer missing columns (#169)#174
shivamlalakiya merged 2 commits into
PhilanthroPy-Project:mainfrom
Larslllllll:test/wealth-percentile-missing-column

Conversation

@Larslllllll

Copy link
Copy Markdown
Contributor

Summary

Add two unit tests to tests/test_preprocessing.py that pin the behaviour of
WealthPercentileTransformer when wealth columns contain missing values.

Changes

  • tests/test_preprocessing.py — New TestWealthPercentileTransformer class with:
    • test_all_missing_column_yields_nan_ranks: Pins the all-NaN column branch:
      the guard stores an empty reference array, transform returns NaN ranks,
      output width is stable, and no warning is raised.
    • test_partially_missing_column_ranks_only_observed_values: Pins the
      partially-NaN column branch: observed values get numeric ranks, NaN input
      rows get NaN ranks.
  • CHANGELOG.md — Added entry under [Unreleased] > Added.
  • CONTRIBUTORS.md — Added self.

Closes

Closes #169.

…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.
@shivamlalakiya
shivamlalakiya enabled auto-merge (squash) September 6, 2026 00:13
@shivamlalakiya
shivamlalakiya merged commit 5258346 into PhilanthroPy-Project:main Sep 6, 2026
15 checks passed
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

@Larslllllll thanks again for #174. Your edge-case tests on WealthPercentileTransformer are what set up the fix that landed in #179 a couple of days later: the tests pinned down what the missing-column behaviour actually was, which is what made it obvious that the behaviour was wrong.

I also owe you a note about #176. You and another contributor solved the same four-line problem on the same evening and I closed one of them, which cost somebody an 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. #187 is merged now and adds a claim-before-PR rule to CONTRIBUTING.md, so issues get assigned before anyone writes code.

Which leads to the question I'm asking everyone who's contributed here. When you set the repo up, what was the most annoying part? Install, the test suite, working out where the code lived, the CI gates, the duplicate-PR mess above, anything at all. There's a thread for it here: #188, or open an issue if it's concrete enough to fix.

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.

test: WealthPercentileTransformer returns all-NaN ranks for an all-missing wealth column, and nothing checks it

2 participants