Skip to content

Fix top_x_pct_share tie handling and zero-percent edge - #288

Merged
MaxGhenis merged 1 commit into
mainfrom
fix/top-x-pct-share-ties
Apr 17, 2026
Merged

Fix top_x_pct_share tie handling and zero-percent edge#288
MaxGhenis merged 1 commit into
mainfrom
fix/top-x-pct-share-ties

Conversation

@MaxGhenis

Copy link
Copy Markdown
Collaborator

Summary

top_x_pct_share used a threshold + >= slice:

threshold = self.quantile(1 - top_x_pct)
self[self >= threshold].sum() / self.sum()

This overstated the share whenever rows tied at the threshold — with constant values every call returned 1.0 regardless of top_x_pct, and top_x_pct_share(0) returned the share of the max bucket instead of 0. Downstream bottom_x_pct_share, top_10/1/0.1_pct_share, bottom_50_pct_share, and t10_b50 all inherited the bug.

Fix: sort by value ascending, cumulate weight, locate the cutoff row where the bottom 1 - top_x_pct of weight ends, and split that row's weight proportionally — the standard wealth-share algorithm.

Reproduction

import microdf as mdf

mdf.MicroSeries([5] * 10, weights=[1] * 10).top_x_pct_share(0.1)  # was 1.0, now 0.1

s = mdf.MicroSeries(list(range(1, 11)), weights=[1] * 10)
s.top_x_pct_share(0.0)  # was 10/55, now 0.0
s.top_x_pct_share(1.0)  # now 1.0

Test plan

  • Existing 51 tests still pass
  • New test_top_x_pct_share_handles_ties_and_edges covers:
    • Constant values (every p returns exactly p)
    • Range 1..10 (closed-form checks against p/55)
    • Unequal ties
    • bottom_x_pct_share complementation
    • Downstream top_10/top_50 still match

@MaxGhenis MaxGhenis left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self-review. Verified:

  • _weighted_top_share algorithm: sort ascending, cumulate weights, find cutoff index via searchsorted(cum_w, target_bottom, side='right'), split the cutoff row's weight proportionally. I hand-computed the constant-series case ([5]*10, p=0.1target=9, k=9, top_sum=5*1=5, share=0.1) and the unequal-ties case ([1,1,10,10], p=0.5target=2, k=2, top_sum=10 + 10*(3-2)=20, share=20/22≈0.909). Both match expected.
  • Edge cases documented and tested: p<=0 → 0.0, p>=1 → 1.0, all-zero series → np.nan. Constant-series returns exactly p for any p ∈ [0, 1].
  • Algorithm is the standard wealth-share / Lorenz-curve approach; downstream bottom_x_pct_share, top_10/1/0.1_pct_share, bottom_50_pct_share, t10_b50 inherit correctly since they all delegate to top_x_pct_share.
  • Docstring clarifies the tie-proportional semantics. Also fixed the bottom_x_pct_share docstring bug (said "top %" — now "bottom %").
  • Tests include constant, 1..10 closed-form (top_x_pct_share(p) = floor(n*p) last rows summed proportionally), unequal ties, and downstream helpers.
  • CI green Py 3.9-3.13.

The previous implementation used

    threshold = self.quantile(1 - top_x_pct)
    self[self >= threshold].sum() / self.sum()

which overstated the share whenever rows tied at the threshold: with
constant values every call returned 1.0 regardless of top_x_pct, and
top_x_pct_share(0) returned the share of the max bucket instead of 0.

Fix: sort by value ascending, cumulate weight, locate the cutoff row
where the bottom 1 - top_x_pct of weight ends, and split that row's
weight proportionally — matching the standard wealth-share algorithm.

Reproduction:
  MicroSeries([5]*10, weights=[1]*10).top_x_pct_share(0.1)
  # was 1.0, now 0.1

Downstream bottom_x_pct_share, top_10_pct_share, top_1_pct_share,
top_0_1_pct_share, bottom_50_pct_share, and t10_b50 all inherit the
fix.
@MaxGhenis
MaxGhenis force-pushed the fix/top-x-pct-share-ties branch from 5897c6a to 9db55ad Compare April 17, 2026 16:17
@MaxGhenis
MaxGhenis merged commit 05bf40d into main Apr 17, 2026
8 checks passed
@MaxGhenis
MaxGhenis deleted the fix/top-x-pct-share-ties branch April 17, 2026 16:17
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.

1 participant