Skip to content

fix(federated): use analytics.ts's median for the peer benchmark, not nearest-rank - #9839

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
shin-core:fix/federated-median-estimator-9645
Jul 29, 2026
Merged

fix(federated): use analytics.ts's median for the peer benchmark, not nearest-rank#9839
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
shin-core:fix/federated-median-estimator-9645

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

refreshFederatedBenchmarkCache (src/orb/federated-benchmark.ts) computed the peer "median merge precision" with percentile(sorted, 50) — a nearest-rank estimator that, on an even-sized peer set, returns a single middle element rather than the mean of the two middle values. computeFleetAnalytics computes the fleet median with median (which averages the two middle values), so the two halves of the same benchmark used different estimators and could disagree for identical data.

The fix

  • Export median from src/orb/analytics.ts and call it in refreshFederatedBenchmarkCache, deleting the now-redundant local .sort(...) (median sorts a copy internally). The federated peer half and the fleet aggregation now share one estimator by construction.
  • Document the two contracts side by side: median sorts internally and averages the two middle values at even n; percentile requires an already-sorted input and does not sort (its two call sites, cycleP50Ms/cycleP95Ms, are named).
  • No change to percentile's p=50 behaviour, so cycleP50Ms/cycleP95Ms are unchanged — they are percentiles, not medians.

Tests (test/unit/federated-benchmark.test.ts)

  • Two peers with mergePrecision 0.4 and 0.8peerMedianMergePrecision 0.6 (the mean of the two middle values), where the old nearest-rank returned 0.4 (fails on main).
  • Updated the existing "computes the median" case: peers 0.5 and 0.9 (untrusted one rejected) → 0.7, not the old nearest-rank 0.5.
  • The existing cycleP50Ms/cycleP95Ms fixtures (test/unit/orb-analytics.test.ts) still pin their exact values — the percentile path is unmoved.

Validation

  • npm run typecheck green; the federated-benchmark + orb-analytics suites green.
  • Diff coverage on both changed src files is 100%.
  • git diff --check <base> HEAD clean; no schema/migration change.

Closes #9645

… nearest-rank

`refreshFederatedBenchmarkCache` computed the peer "median" with
`percentile(sorted, 50)` — a nearest-rank estimator that, on an even-sized peer
set, returns one middle element rather than the mean of the two. `computeFleetAnalytics`
uses `median` (which averages the two middle values), so the peer benchmark and the
fleet aggregation could disagree for the same data.

Export `median` from analytics.ts and call it (it sorts internally, so the local
`.sort()` is removed), making the two halves share one estimator by construction.
Document the two contracts side by side: `median` sorts a copy and averages at even
n; `percentile` requires a pre-sorted input and does not sort (its call sites,
cycleP50Ms/cycleP95Ms, are named). Those percentiles are unchanged — they are
percentiles, not medians.

Closes JSONbored#9645
@shin-core
shin-core requested a review from JSONbored as a code owner July 29, 2026 13:09
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 13:19:01 UTC

3 files · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This is a narrow, correct bug fix: it unifies the peer-benchmark median with the fleet's `median` function by exporting it from analytics.ts and swapping the federated-benchmark.ts call site off the nearest-rank `percentile(...,50)`. The trace is verifiable end-to-end — the old code sorted peer precisions and called `percentile(sorted,50)`, which returns the upper-middle value on even n, while `median` correctly averages the two middle values; the updated tests pin this with a concrete 0.4/0.8→0.6 case and the corrected 0.5/0.9→0.7 case. `percentile`'s own p=50 call sites (`cycleP50Ms`/`cycleP95Ms`) are untouched and remain correct since those are genuinely percentiles, not medians.

Nits — 3 non-blocking
  • The 'magic number' flags in the external brief (analytics.ts:179,190) are pre-existing `50` literals for `percentile(..., 50)` inside the new JSDoc/example text, not new logic — not worth a constant.
  • src/orb/federated-benchmark.ts's redundant `.sort()` removal is correct since `median` sorts internally, but it's worth a one-line confirmation comment noting the removed sort is now redundant (already partially done in the inline comment).
  • Consider a quick regression test asserting `median` and `percentile(sorted,50)` diverge on a canonical even-length input directly in orb-analytics.test.ts, so any future accidental revert of the federated-benchmark.ts call site is caught even without exercising the federated path.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9645
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 71 registered-repo PR(s), 52 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 71 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: minor
Linked issue satisfaction

Addressed
The diff exports and switches to analytics.ts's median in refreshFederatedBenchmarkCache, removes the redundant sort, rewrites the misleading comment to correctly describe the shared estimator, adds precondition/behavior docs to percentile and median, and includes the mandated 0.4/0.8→0.6 regression test plus an updated existing test reflecting the new 0.7 result while leaving percentile/cycleP50M

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: not available
  • Official Gittensor activity: 71 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Await review-lane availability.
  • Then work through the remaining 1 step in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.09%. Comparing base (f881645) to head (174fe36).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9839      +/-   ##
==========================================
+ Coverage   79.04%   79.09%   +0.04%     
==========================================
  Files         281      283       +2     
  Lines       58409    58579     +170     
  Branches     6698     6748      +50     
==========================================
+ Hits        46171    46334     +163     
- Misses      11955    11959       +4     
- Partials      283      286       +3     
Flag Coverage Δ
backend 95.88% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/orb/analytics.ts 100.00% <ø> (ø)
src/orb/federated-benchmark.ts 86.27% <100.00%> (ø)

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 2adaed6 into JSONbored:main Jul 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

orb(federated): the peer "median" uses nearest-rank percentile

1 participant