Skip to content

fix(aggregation): Use squared distances for Krum scores - #835

Merged
ValerianRey merged 4 commits into
SimplexLab:mainfrom
AryanRG:fix-krum-squared-distances
Oct 31, 2026
Merged

ValerianRey merged 4 commits into
SimplexLab:mainfrom
AryanRG:fix-krum-squared-distances

Conversation

@AryanRG

@AryanRG AryanRG commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Krum currently sums Euclidean distances, but the paper defines scores using squared Euclidean distances. The rankings can differ: with rows [[3,0],[0,7],[2,5],[5,0],[6,7]] and n_byzantine=1, Krum selects [3,0] instead of [2,5].

Remove the square root while retaining the roundoff clamp. Add regressions for Krum, Multi-Krum, selection boundaries, and an autograd/SGD step through both Gramian computation modes.

Validated the full CPU unit suite, focused float32/float64 aggregation and autograd tests, Ruff, full type checking, and wheel/sdist contents plus wheel-import smoke tests. The unit suite retains its existing skips and expected failures; CUDA and the full CI matrix were not run locally.

@AryanRG

AryanRG commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Could a maintainer add cc: fix? The title check requires it, and I don't have permission to set labels.

@ValerianRey ValerianRey added the cc: fix Conventional commit type for bug fixes of the actual library (changes to src). label Oct 30, 2026
@github-actions github-actions Bot changed the title fix(aggregation): Use squared distances for Krum scores fix: Use squared distances for Krum scores Oct 30, 2026
@github-actions github-actions Bot changed the title fix: Use squared distances for Krum scores fix(aggregation): Use squared distances for Krum scores Oct 30, 2026
@ValerianRey

Copy link
Copy Markdown
Member

/opencode:Plan please review this. In particular:

  • find the official implementation of Krum and see what they did
  • read the paper and verify that the PR's claim is correct
  • if this bug was introduced after the commit adding Krum, find out at which commit

@opencode-agent

Copy link
Copy Markdown
Contributor

APIError: This API key has expired. Create a new API key in OpenCode Console and replace it in your client or secret store. Signing in again will not renew this key.

github run

@ValerianRey

Copy link
Copy Markdown
Member

APIError: This API key has expired. Create a new API key in OpenCode Console and replace it in your client or secret store. Signing in again will not renew this key.

github run

@PierreQuinton

@AryanRG

AryanRG commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

@ValerianRey the authors' Krum repository points to AggregaThor. Its implementation computes squared distances and sums the closest n-f-2, matching section 4 of the paper.

In TorchJD, unsquared scoring was already present in the initial commit: torch.cdist followed by a sum. It predates the later refactors and clamp change. Squaring preserves each row's nearest-neighbor order, but can change which row has the lowest total score, as the PR's regression demonstrates.

@ValerianRey

Copy link
Copy Markdown
Member

@ValerianRey the authors' Krum repository points to AggregaThor. Its implementation computes squared distances and sums the closest n-f-2, matching section 4 of the paper.

In TorchJD, unsquared scoring was already present in the initial commit: torch.cdist followed by a sum. It predates the later refactors and clamp change. Squaring preserves each row's nearest-neighbor order, but can change which row has the lowest total score, as the PR's regression demonstrates.

Thanks for the confirmation, and thanks a lot for catching this. Seems like a bad mistake I've made since the first day of implementing this.

I'll improve a bit the unit tests and merge this!

ValerianRey and others added 2 commits October 31, 2026 14:38
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ValerianRey
ValerianRey merged commit c26fa16 into SimplexLab:main Oct 31, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cc: fix Conventional commit type for bug fixes of the actual library (changes to src). package: aggregation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants