Sweep index size and negative ratio in run_grid_search - #34
Open
wxiao0421 wants to merge 1 commit into
Open
Conversation
This was referenced Jul 29, 2026
wxiao0421
force-pushed
the
feat/index-subsample
branch
from
July 29, 2026 20:07
ded6bd5 to
839e2ee
Compare
wxiao0421
force-pushed
the
feat/grid-search-index-axes
branch
from
July 29, 2026 20:07
335240b to
21bec3a
Compare
This was referenced Jul 29, 2026
wxiao0421
force-pushed
the
feat/index-subsample
branch
from
July 29, 2026 23:49
839e2ee to
2287470
Compare
Index size is the highest-leverage knob for tuning, but run_grid_search could only sweep top_k and min_score. With subsample() available, size and ratio can be swept too, and cheaply: reshaping the index costs about a millisecond, while re-scoring costs seconds. The loop is organised around what each setting costs. Size and ratio reshape the index (cheap) and become an outer loop; top_k forces a re-score (expensive); thresholds and aggregators stay free on the cached scores. Rows now carry the requested n_positive and neg_to_pos_ratio plus the actual n_positive_actual and n_negative_actual counts. The actual counts matter because a request is clipped when the index is smaller than asked for - without them two rows can look identical while describing different indices. Backward compatible: both new arguments default to None, meaning one pass using the index exactly as given. subsample() is not called at all in that case, so any index-like object keeps working and existing behaviour is unchanged. Also fixes a latent NameError: simulation.py referenced LOG without importing logging or defining it. Nothing had triggered it because the module had no logging calls until this one. Documented in the docstring, and worth calling out for reviewers: the cost is len(sizes) x len(ratios) x len(top_k) scoring passes, and the observation texts are re-encoded on every pass even though their embeddings do not depend on the index at all. Letting callers pass pre-computed sample embeddings would collapse that to roughly one encoding pass. Deliberately out of scope here. Adds 5 tests covering the default no-subsample path, both axes together and separately, clipped counts, and the per-configuration progress logging. Co-authored-by: Cursor <cursoragent@cursor.com>
wxiao0421
force-pushed
the
feat/grid-search-index-axes
branch
from
July 29, 2026 23:50
21bec3a to
613c40c
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
subsample()from #33 is the enabler; this is the payoff. Index size is the highest-leverage knob for tuning, butrun_grid_searchcould only sweeptop_kandmin_score. Now it can sweep size and ratio too:The loop is organised around what each setting actually costs, which is the structure the function already had — the new axes simply belong on the expensive side, as an outer loop:
Verified end to end against the real
examples/hate_speech_modelindex: a 2×2×2 sweep produced 96 rows across 4 distinct index configurations, with progress logged per configuration.Requested vs actual counts
Each row gains four keys:
n_positiveandneg_to_pos_ratio(what you asked for) andn_positive_actual/n_negative_actual(what you got).The actual counts are not redundant. A request is clipped when the index is smaller than asked for, so without them two rows can look identical while describing genuinely different indices:
Backward compatibility
Both new arguments default to
None, meaning "one pass, use the index exactly as given." In that casesubsample()is never called, so any index-like object keeps working, including test stubs that do not implement it. The pre-existing grid-search test passes untouched.The new columns are still emitted in the default case (as
None), so the result table has a consistent shape whichever way it is called.Drive-by fix
simulation.pyreferencedLOGbut never importedloggingor defined it. That was a latentNameError— nothing had triggered it because the module contained no logging calls until this PR added one. Caught by the new progress-logging test.A cost issue reviewers should know about
The number of scoring passes is
len(n_positive_values) × len(neg_to_pos_ratios) × len(top_k_values). The 7×7 grid the internal pipeline uses, with threetop_kvalues, is 147 full scoring passes.Here is the subtle part: the observation texts are re-encoded on every single pass, and their embeddings do not depend on the index at all.
score_groupshands raw text tocalculate_rare_class_affinity, which encodes it internally each time. So most of that 147× cost is recomputing identical numbers.The library authors already anticipated this, at
sentinel_local_index.py:Deliberately out of scope here. Accepting optional pre-computed
sample_embeddingsand caching them once inrun_grid_searchwould cut a 147-pass sweep to roughly one encoding pass plus 147 cheap nearest-neighbour searches. That deserves its own PR. For now the cost is documented in the docstring, and progress is logged per index configuration so a long sweep cannot be mistaken for a hang.What breaks if this is wrong
subsample()is never called when the new axes are unused.top_kloop instead of outside it, the sweep would still produce correct-looking numbers but would rebuild the index needlessly on every scoring pass, quietly throwing away the speedup this PR exists to deliver. Asserted directly: 4 configurations produce exactly 4subsample()calls, not 8.Test plan
5 new tests.
subsample(), and still emits the new columns asNonesubsample()called once per(size, ratio)pair and not once pertop_kindex_seedis forwarded to everysubsample()callpytest tests/— 87 passed (82 before this PR)flake8 --config=.flake8clean on both changed filesdocs/check_docs_sync.pyreports in syncsubsample(), not a stubNote on CI:
.github/workflows/test.ymlonly triggers on pull requests targetingmain, so stacked PRs show no checks. Run locally against Python 3.10; the full 3.10/3.11/3.12 matrix runs once the stack retargets tomain.