Repository navigation
fix: spread the quantile sample remainder instead of dropping it - #192
Merged
SrGonao merged 1 commit intoNov 2, 2026
Conversation
`split_quantiles` took `n_samples // n_quantiles` from every quantile, so a request that didn't divide evenly came back short and one smaller than `n_quantiles` came back empty. `--n_examples_test 5` with the default `--n_quantiles 10` asked for five scoring examples and produced none, with no warning: the per-quantile count was 0, which is never fewer than the examples available, so the "fewer than" log never fired either. Each quantile's count now comes from the difference of two cumulative counts, which sums to `n_samples` exactly and puts the remainder in quantiles spaced across the range rather than bunched at the top. A request that already divided evenly, every default among them, takes the same examples it did before.
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.
What this fixes
split_quantiles(delphi/latents/samplers.py) computes one per-quantile count forthe whole loop:
So
n_samplesis honoured only when it divides byn_quantiles. Otherwise theremainder is dropped, and when
n_samples < n_quantilesthe count is 0 and thefunction returns nothing at all.
Nothing warns in that case. The shortfall log is guarded by
len(quantile) < samples_per_quantile, and no quantile has fewer than zeroexamples, so it never fires.
Measured against the current
mainwith 1000 examples:n_samplesn_quantilesThe sharp edge is
SamplerConfig.n_examples_test=50andn_quantiles=10are thedefaults and divide cleanly, so this is invisible on a default run. Pass
--n_examples_test 5, or raise--n_quantilespastn_examples_test, and the latentis explained and then scored on an empty test set.
--n_examples_train 25quietlyshows the explainer 20 examples.
Fix
Take each quantile's count as the difference of two cumulative counts:
The counts sum to
n_samplesexactly, and then_samples % n_quantilesextra samplesland in quantiles spaced across the range rather than bunched at the top, which would
bias the sample toward the highest-activating examples. For
n_samples=25, n_quantiles=10that is
[2, 3, 2, 3, 2, 3, 2, 3, 2, 3]; for5, 10it is[0, 1, 0, 1, 0, 1, 0, 1, 0, 1].A request that already divided evenly gets the identical count per quantile, so every
default config and every config in
tests/andtests/e2e.pysamples exactly what itsampled before.
train_type="mix"now reachesn_trainas well: with the defaults itwas taking 38 of the 40 it asked for.
Which quantile an example belongs to doesn't change. The quantile slices and the
excludecheck are untouched.Tests
In
tests/test_latents/test_sampler.py:test_split_quantiles_returns_as_many_as_asked_for, parametrized over the pairs inthe table above. The six non-dividing pairs fail on
maintest_split_quantiles_samples_stay_in_their_own_quantile— the remainder changes howmany come from a quantile, not which quantile an example is in
test_fewer_test_examples_than_quantiles— the symptom throughsampler:n_examples_test=5withn_quantiles=10leavesrecord.testempty onmainThe existing
test_simple_cacheassertslen(record.test) <= n_examples, which is whyn_examples=5, n_quantiles=10passes today on an empty result. I left that assertionalone rather than tightening someone else's test in this PR.
The 1000-example fixture the file already builds is now a small helper, since four tests
need it.
CI, and why these tests can't run yet
mainhas been red since 30 Oct, stopping during collection before any test runs:One of the five modules is
tests/test_latents/test_sampler.py, which is where these testslive, so this PR's tests cannot run until #185 lands. Nothing here touches that.
I ran the build on my fork twice to separate the two. This branch alone reproduces the same
collection error, with
ruff checkclean and none of the repo's 64 pyright errors in eitherfile I touched. With #185 cherry-picked on top of this branch the suite is green:
(run 41433109283), against
278 passedon #185's own run(41318281754). The difference
is exactly the nine tests added here, and none of the existing 278 broke.
Happy to rebase this on #185 whenever that suits you.
What I ran locally
ruff checkon both files, cleanblackon both files (pre-commit's pinned 26.3.0)pytest. Saying thatrather than reporting a pass I didn't see. The count arithmetic and the slicing are plain
Python, so I ran both versions of the function side by side over
n_samples0 to 60 acrossn_quantilesin 1, 2, 3, 7, 10, 23: every total matches the request, no sample leaves itsquantile, and
excludestill holds. CI is the real run.