Fix group-cluster serialization: unfold base-cluster IDs to participant IDs (squashed into #2431)#2433
Closed
jucor wants to merge 9 commits into
Closed
Fix group-cluster serialization: unfold base-cluster IDs to participant IDs (squashed into #2431)#2433jucor wants to merge 9 commits into
jucor wants to merge 9 commits into
Conversation
This was referenced Mar 10, 2026
jucor
changed the title
jucor
changed the title
5 tasks
jucor
changed the title
jucor
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where two-level clustering output exposed internal base-cluster IDs instead of participant IDs in group-cluster members. It introduces a _unfolded_group_clusters() helper that expands base-cluster IDs to participant IDs and applies it across all downstream consumers and serialization paths.
Changes:
- Added
_unfolded_group_clusters()helper inconversation.pyand updated 5 downstream consumers (conv_repness,participant_stats,group_votes,to_dict,get_full_data,to_dynamo_dict) to use unfolded participant IDs. - Tightened Clojure comparison thresholds (Jaccard 0.95→0.99, distribution tolerance 0.05→0.01, exact comment priority matching) and updated documentation to reflect both Python and Clojure now use two-level clustering.
- Added
test_serialization_unfolding.pywith 8 tests covering all serialization paths, and removedxfailmarkers from tests that now pass with the fix.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
delphi/polismath/conversation/conversation.py |
Core fix: added _unfolded_group_clusters() and updated all serialization/consumer call sites |
delphi/tests/test_serialization_unfolding.py |
New TDD tests verifying serialized cluster members are participant IDs |
delphi/tests/test_repness_smoke.py |
Updated to use _unfolded_group_clusters(), removed xfail on test_repness_structure |
delphi/tests/test_legacy_clojure_regression.py |
Updated clustering comparison to unfold both sides, tightened thresholds, removed xfail |
delphi/polismath/regression/clojure_comparer.py |
Updated docstring to reflect both implementations use two-level clustering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Convert group clusters (unfolded: base-cluster IDs → participant IDs) | ||
| base_clusters = [] | ||
| for cluster in self.group_clusters: | ||
| for cluster in self._unfolded_group_clusters(): |
jucor
changed the title
jucor
changed the title
jucor
changed the title
Creates generate_cold_start_clojure.py to generate fresh cold-start Clojure reference data for fair Python vs Clojure comparison. The script: - Stops if math worker is running (prevents conflicts) - Backs up existing math_main row - Deletes row to force cold-start (load-or-init creates fresh new-conv) - Runs Clojure computation via Docker - Extracts cold-start math blob - Restores original row automatically Key features: - Support for --all flag to process all datasets - Support for --include-local flag for local datasets - Automatic zid lookup from report_id via reports table - Loads configuration from polis-kmeans/.env (DATABASE_URL) Test infrastructure updates: - datasets.py now prefers cold-start blobs when available - Added has_cold_start_blob field to DatasetInfo - get_dataset_files() uses cold-start blob by default Documentation updates: - Comprehensive usage guide in SESSION_HANDOFF_KMEANS.md - Commands to find and verify cold-start blobs - Configuration requirements and setup instructions Reference data: - Generated cold-start blobs for biodiversity and vw datasets - Tests will now use these for fair cold vs cold comparison Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…add visualization with PCA sign flip detection Cold-start generation (generate_cold_start_clojure.py): - Rewrite using "conversation replay" approach that works with Clojure poller design - Creates temporary conversation with fresh zid, copies votes with fresh timestamps - Runs poller with MATH_ZID_ALLOWLIST to only process the replayed conversation - Automatically cleans up all temporary data (math tables, votes, conversation) - Add bash wrapper script (generate_cold_start.sh) that stops math containers first Visualization (visualize_cluster_comparison.py): - Add PCA sign flip detection by comparing component correlations - Apply sign corrections to base cluster centers before visualization - Fix convex hull rendering to show outlines for both datasets in overlay view - Include sign_flips in metrics JSON output Documentation: - Update SESSION_HANDOFF_KMEANS.md with new approach and remove "BROKEN" warnings - Document the conversation replay workflow and cleanup behavior Regenerate cold-start blobs for biodiversity and vw datasets. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Cold-start generator (generate_cold_start_clojure.py): - Add --pause-math to pause/resume workers instead of stopping - Add --verbose/-v for real-time Clojure poller output - Support multiple datasets as arguments - Use fast INSERT...SELECT for vote copying (was executemany) - Handle duplicate votes with DISTINCT ON - Remove shell wrapper (functionality now in Python script) Cluster visualizer (visualize_cluster_comparison.py): - Add --all option for processing all datasets - Synchronize X/Y axis limits in side-by-side plots - Print full absolute paths for generated PNGs - Support multiple datasets as arguments Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Deferred from commit 18ad361 — the test_datasets.py changes depend on the has_cold_start_blob field introduced in datasets.py by the cold-start tooling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…am consumers Group clusters store base-cluster IDs in 'members' (matching Clojure's two-level clustering architecture), but downstream functions (conv_repness, participant_stats, group_votes) need participant IDs to join against the vote matrix. Add _unfolded_group_clusters() helper (equivalent to Clojure's clusters/group-members) and use it in all 5 call sites: - _compute_repness - _compute_participant_info - _compute_group_votes - to_dict group-votes - to_dynamo_dict group_votes Also re-record golden snapshots and remove xfail from test_repness_structure (now passes with correct unfolding). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- test_group_clustering: unfold Python clusters via _unfolded_group_clusters() (both sides now use two-level clustering), tighten thresholds to 0.99 Jaccard / 0.01 distribution tolerance, require overall_match - test_comment_priorities: require exact match (1e-6 tolerance) for all comment IDs instead of 70% at 20% tolerance - clojure_comparer: fix docstring to reflect Python also uses two-level clustering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
to_dict(), get_full_data(), and to_dynamo_dict() were writing self.group_clusters directly, whose 'members' contain base-cluster IDs (integers 0..N). Downstream consumers (group_data.py, Clojure compat, client apps) expect participant IDs. The internal helper _unfolded_group_clusters() already existed and was used for repness/participant_info/group_votes computation, but the five serialization sites were missed. Fix all five sites to unfold before serializing: - to_dict(): group-clusters, group_clusters, base-clusters - get_full_data(): group_clusters - to_dynamo_dict(): base_clusters / group_clusters Add test_serialization_unfolding.py (TDD: 6 tests fail on the bug, 8/8 pass after fix) using real recompute() pipeline output. Re-record golden snapshots to reflect the corrected output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jucor
force-pushed
the
jc/cold-start-tooling
branch
from
ab15ae0 to
04398a1
Compare
jucor
force-pushed
the
jc/participant-id-unfolding
branch
from
b2f145d to
48afbe4
Compare
jucor
changed the title
Delphi Coverage Report
|
jucor
changed the title
jucor
changed the title
jucor
changed the title
jucor
force-pushed
the
jc/cold-start-tooling
branch
from
04398a1 to
ad6cf37
Compare
Collaborator
Author
|
Squashed into #2431 (Stack 3, jc/two-level-clustering). The unfolding fix is now part of the two-level clustering PR that introduced the code it fixes. |
jucor
changed the title
Collaborator
Author
|
Thanks @ballPointPenguin ! |
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
Fixes a bug where two-level clustering output exposed internal base-cluster IDs instead of participant IDs to downstream consumers and serialization.
Changes
_unfolded_group_clusters()helper inconversation.py: Expands group-clustermembersfrom base-cluster IDs to participant IDs (equivalent to Clojure'sclusters/group-members).conv_repness,participant_stats,group_votes(in_compute_representativeness,_compute_participant_info,to_dict).to_dict()(group-clusters, group_clusters, base-clusters),get_full_data()(group_clusters),to_dynamo_dict()(base_clusters/group_clusters).test_serialization_unfolding.py(TDD: 6/8 fail on the bug, 8/8 pass after fix).Test plan
🤖 Generated with Claude Code