Skip to content

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

Open
AryanRG wants to merge 1 commit into
SimplexLab:mainfrom
AryanRG:fix-krum-squared-distances
Open

AryanRG wants to merge 1 commit into
SimplexLab:mainfrom
AryanRG:fix-krum-squared-distances

Conversation

@AryanRG

@AryanRG AryanRG commented Oct 30, 2026

Copy link
Copy Markdown

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
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
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.

This branch has not been deployed

No deployments
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