Fix top_x_pct_share tie handling and zero-percent edge - #288
Merged
Conversation
MaxGhenis
commented
Apr 17, 2026
MaxGhenis
left a comment
Collaborator
Author
There was a problem hiding this comment.
Self-review. Verified:
_weighted_top_sharealgorithm: sort ascending, cumulate weights, find cutoff index viasearchsorted(cum_w, target_bottom, side='right'), split the cutoff row's weight proportionally. I hand-computed the constant-series case ([5]*10, p=0.1→target=9, k=9, top_sum=5*1=5, share=0.1) and the unequal-ties case ([1,1,10,10], p=0.5→target=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 exactlypfor anyp∈ [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_b50inherit correctly since they all delegate totop_x_pct_share. - Docstring clarifies the tie-proportional semantics. Also fixed the
bottom_x_pct_sharedocstring 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
force-pushed
the
fix/top-x-pct-share-ties
branch
from
April 17, 2026 16:17
5897c6a to
9db55ad
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
top_x_pct_shareused a threshold +>=slice:This overstated the share whenever rows tied at the threshold — with constant values every call returned
1.0regardless oftop_x_pct, andtop_x_pct_share(0)returned the share of the max bucket instead of 0. Downstreambottom_x_pct_share,top_10/1/0.1_pct_share,bottom_50_pct_share, andt10_b50all inherited the bug.Fix: sort by value ascending, cumulate weight, locate the cutoff row where the bottom
1 - top_x_pctof weight ends, and split that row's weight proportionally — the standard wealth-share algorithm.Reproduction
Test plan
test_top_x_pct_share_handles_ties_and_edgescovers:preturns exactlyp)p/55)bottom_x_pct_sharecomplementationtop_10/top_50still match