fix(monitor): coordinated sampling allocates on per-sid (agg-id) freq f_i, not total rate - #377
Merged
Merged
Conversation
… f_i, not total rate coordinator.rs::allocate_p() passed the edge `rate` vector as BOTH the rate and the freq argument to allocate_sample_rates(), so √(f_i/rate_i)=1 for every edge and the KKT allocation collapsed to a uniform p — all differentiation was left to the ε-floor (rate-only). The MonitorReport already carries `known_value`: the monitored functional's per-edge value, i.e. the frequency of the monitored series id (the cms_point sid — internally the CMS `key`) / the edge's sum contribution, identified by the report's aggregation_id. That, not the total update rate, is the correct f_i. Feed it as the freq vector so the allocation is genuinely p_i ∝ √(f_i/rate_i), evaluated per-aggregation per-edge: an edge carrying more of the monitored sid at equal total rate now keeps a higher p. Edges with no monitored-sid mass (known_value=0) fall back to p=1. New test per_key_freq_differentiates_at_equal_rate pins the freq-driven branch (equal rate, different known_value ⇒ different p, both above the equal rate-floor); the existing skewed-rate (floor-driven) test still passes. 13/13 monitor tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
zzylol
force-pushed
the
fix/coordinator-per-key-freq
branch
from
June 16, 2026 20:53
140fb3d to
1beedb4
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.
Problem
coordinator.rs::allocate_p()passed the edgeratevector as both the rate and the freq argument toallocate_sample_rates():Since
p_i = clamp(√λ·√(f_i/rate_i), 0, 1)andf_i == rate_i, every√(f_i/rate_i) = 1→ the allocation collapses to a uniform p. All edge differentiation was left to the ε-floor (which depends onrateonly). So the headlinep_i ∝ √(f_i/rate_i)coordinated-sampling claim was not actually exercised — the comment even said "freqs proxy = rates (no per-key split available at the coordinator)."Fix
The
MonitorReportalready carriesknown_value— the monitored functional's per-edge value this epoch (thecms_pointkey frequency, or the edge's contribution to the monitored sum), which is the correctf_iand is distinct from the edge's total updaterate. Feed it as the freq vector:Now an edge carrying more of the monitored key at equal total rate keeps a higher
p; an edge with no monitored-key mass (known_value=0) falls back top=1.Tests
per_key_freq_differentiates_at_equal_rate: equal rate, differentknown_value⇒ differentp, both above the equal rate-floor (fails on pre-fix code).skewed_rates_yield_differentiated_sample_p_above_floorstill passes.monitor::coordinatortests green.Found while wiring Fig 9 (coordinated sampling) on the 8-node eval cluster.
🤖 Generated with Claude Code