Skip to content

Fix prop_above_and_below typing and empty concat handling#353

Closed
neuralsorcerer wants to merge 3 commits intofacebookresearch:mainfrom
neuralsorcerer:prop
Closed

Fix prop_above_and_below typing and empty concat handling#353
neuralsorcerer wants to merge 3 commits intofacebookresearch:mainfrom
neuralsorcerer:prop

Conversation

@neuralsorcerer
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings March 2, 2026 19:20
@meta-cla meta-cla bot added the cla signed label Mar 2, 2026
@neuralsorcerer neuralsorcerer added this to the balance 0.17.0 milestone Mar 2, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the prop_above_and_below() function in balance/stats_and_plots/weights_stats.py by:

  1. Replacing the untyped dict[Any, Any] return type with a new PropAboveBelowResult TypedDict.
  2. Adding @overload signatures to let static type checkers narrow the return type based on the return_as_series literal argument.
  3. Fixing a latent bug where pd.concat could be called with an all-None list (when one of below/above is None) by building the pieces list with only non-None Series.

Changes:

  • Introduced PropAboveBelowResult TypedDict and @overload signatures for prop_above_and_below, plus a safe concat fix when only one threshold group is provided
  • Added edge-case tests covering empty threshold iterables, mixed-None threshold groups in dict mode, and all-None both-groups behavior
  • Updated CHANGELOG.md with entries for both the fix and the new tests

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
balance/stats_and_plots/weights_stats.py Introduces PropAboveBelowResult TypedDict, adds @overload signatures, and replaces unsafe pd.concat with filtered-pieces guard
tests/test_stats_and_plots.py Adds test_prop_above_and_below_edge_cases covering empty iterables, mixed-None dict mode, and both-None case
CHANGELOG.md Records the return-path stabilization and expanded test coverage

Two issues were identified:

  1. Second @overload has an unnecessary and misleading default value (line 222): return_as_series: Literal[False] = False declares False as the default, creating ambiguity for Pyre/mypy when the argument is omitted — both overloads now claim to apply. The standard pattern is to omit the default on the non-default overload: return_as_series: Literal[False] (no = False).

  2. Docstring Returns section and below/above parameter descriptions are stale (lines 227–232 area): The Returns: type hint still says pd.Series | dict instead of pd.Series | PropAboveBelowResult | None, and the descriptions for below and above still say "Using None returns None," which is only true when both are None.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@neuralsorcerer neuralsorcerer requested a review from talgalili March 2, 2026 20:03
@meta-codesync
Copy link

meta-codesync bot commented Mar 3, 2026

@talgalili has imported this pull request. If you are a Meta employee, you can view this in D95019739.

@meta-codesync
Copy link

meta-codesync bot commented Mar 3, 2026

@talgalili merged this pull request in ddae91c.

@neuralsorcerer neuralsorcerer deleted the prop branch March 3, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants