Warn when cluster studio sample_size exceeds available clusters (#1026) - #3227
Open
Aleksandrs Drozdovs (aleks-drozy) wants to merge 1 commit into
Conversation
…analytical-services#1026) The DuckDB "Sample sample_size ... out of range" crash from moj-analytical-services#1026 is already fixed on master via SplinkDialect.proportion_sample_sql returning an empty filter for proportion >= 1.0. What was still missing, and what the issue explicitly asks for, is user-facing feedback: previously an oversized sample_size would silently return every cluster with no explanation, which can also mask a too-low clustering threshold or an overly-matchy model. - Add a module logger to cluster_studio.py (matching the convention in expectation_maximisation.py). - In _get_random_cluster_ids, warn when sample_size >= cluster_count and clamp proportion to at most 1.0 so intent is explicit locally. - Mirror a warning in the by_cluster_size and lowest_density_clusters_by_size branches of _get_cluster_ids, which silently return fewer clusters than requested in the same situation. - Add regression tests covering the warning (and its absence when sample_size is satisfiable) for _get_random_cluster_ids. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Addresses #1026.
The original DuckDB crash reported in this issue (
Sample sample_size 133.3 out of range) looks like it's already fixed onmaster—SplinkDialect.proportion_sample_sqlnow returns an emptyWHEREfilter forproportion >= 1.0, so it no longer errors whensample_sizeexceeds the number of available clusters.What's still missing, and what this PR adds, is the actual feedback the issue asks for: previously that case silently returned all clusters with no indication anything was off. Now:
_get_random_cluster_idslogs a warning naming the requestedsample_size, the actual number of distinct clusters, and notes this can indicate an over-low clustering threshold or an overly "matchy" model — then clampsproportiontomin(sample_size / cluster_count, 1.0)so the intent is explicit locally rather than relying on the dialect-level guard._get_cluster_ids'sby_cluster_size/lowest_density_clusters_by_sizebranches (which also returned fewer clusters than requested with no signal) gets a matching warning.Testing: added
test_random_sample_size_exceeds_cluster_count_warnsandtest_random_sample_size_within_cluster_count_no_warningtotests/test_cluster_studio.py, covering both the warning and the "no false-positive warning" case. Existingtest_density_sampleuntouched.ruff check/formatandmypyboth clean.I noticed the issue still carries a several-years-old self-assignment — flagging that here in case it's stale and someone else should take a look, happy to have this superseded if there's already work in progress I didn't find.