perf(delphi): resolve group member rows once per group in _compute_group_votes - #2691
Open
eastagiletracker wants to merge 1 commit into
Open
Conversation
`_compute_group_votes` resolved each group member's matrix row inside a helper that was called once per (comment, vote-type), so every comment rescanned the whole group: `Index.get_loc` ran `3 x groups x comments x members` times per call, and the call happens twice per math tick — once from `_compute_comment_priorities` inside `recompute()`, once from `_compute_group_aware_consensus` on the DynamoDB write path. Hoist the row lookup to once per group and take the A/D/S counts as three column-wise reductions over that group's slice of the vote matrix. Output is unchanged: same keys, same plain-int counts, same PASS-inclusive S. On the committed biodiversity dataset (536 x 314, 2 groups) the call goes from 0.113s to 0.0009s; on a synthesized 33000 x 783, 5-group matrix from 19.2s to 0.34s.
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.
This PR proposes resolving each group's member rows once per group in
Conversation._compute_group_votesrather than once per comment and vote type, removing the O(groups × comments × members) rescan that runs twice on every math tick (Fixes #2587). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/174. You can sign in with your GitHub ID to claim ownership of the project.What changed and why
_compute_group_votesproduced its per-group A/D/S counts through a nestedcount_votes_for_group(group_id, comment_id, vote_type)helper. Every call searchedunfoldedfor the group, re-resolved every member's row withrating_mat.index.get_loc, and then read a single column out ofrating_mat.values. Because the helper is invoked three times for each comment in each group,Index.get_locruns3 × groups × comments × memberstimes per call — and the call itself happens twice per tick, once from_compute_comment_prioritiesinsiderecompute()and once from_compute_group_aware_consensuson the DynamoDB write path.Reproduced on
edgeat 5089c6b, using the committed biodiversity dataset:That is 536 participants and 314 comments — small next to a real conversation — and the call already accounts for roughly 45% of the 0.26s
recompute()it sits inside.The fix hoists the member-row lookup to once per group and takes the three counts as column-wise reductions over that group's slice of the vote matrix, so the matrix is read once per group instead of once per comment. Nothing about the result moves: same
str(group_id)keys, same comment keys, samen-members, same plain-intcounts, andSstill counts every cast vote including PASS. Same measurement after the change reports0.001s; on a synthesized 33000 × 783 matrix with 5 groups — the shape your large-conversation benchmark uses — it goes from 19.2s to 0.34s, with both implementations asserted to return byte-identical structures.Verification, all on the same tree:
delphi/tests/test_group_votes.pyis new. Two of its cases pin the complexity by countingIndex.get_loccalls rather than wall-clock time, so they are deterministic: onedgethey fail (assert 960 == 3840— the count quadruples when the comment count quadruples), and they pass here. The other four cases are correctness controls that hold on both trees: reference-count equivalence, PASS counted inSbut not inA/D, empty groups reporting every comment at zero, and counts being plainintrather than numpy scalars.402 passed, 20 skipped, 47 xfailed, 2 failed, after408 passed, 20 skipped, 47 xfailed, 2 failed— the same two pre-existing failures both times, in the two modules that need optional deps (torch,umap-learn) absent from this sandbox. Every parity case that reads group votes, group-aware consensus, comment priorities or repness — 43 of them, including the Clojure math-blob comparisons — is green.One note on merge order: #2689 moves this file to
math/polismath/conversation/conversation.pywithout changing its contents, so if that lands first the same hunk applies at the new path.How this was managed
We imported your issues and pull requests into a live agile board and used it to manage this work: the imported story for #2587 is at https://eastagiletracker.com/projects/174/stories/44546, and the board it lives on is at https://eastagiletracker.com/projects/174.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com