Fix tied nearest neighbors merging a clade twice - #356
Merged
josevalim merged 3 commits intoAug 22, 2026
Conversation
The chain only cycles back on its immediate predecessor while the distance along it strictly decreases. Ties break that: argmin can return a different clade that is tied for nearest, the chain walks back onto an entry it already holds, and the duplicate outlives the merge that consumed it. It is then merged a second time, after it is already gone, so a clade ends up under two parents, the sizes stop adding up, and the tree never closes over every point. End the chain whenever the previous entry is a nearest neighbor of the tip, not only when it is the one argmin returns.
Merged
…tive Merged clades are masked out of the matrix rather than blanked, so their rows keep stale values that ward's update still reads. The term can then come out negative for a dead column, which raises on the binary backend and is a silent NaN under EXLA.
An infinite coordinate puts both infinity and NaN in the distance matrix, so the chain can keep extending without finding a mutual pair. Walking past the end of its buffer raised instead of reporting the merges it could not make.
Contributor
|
💚 💙 💜 💛 ❤️ |
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.
Hierarchical.fitcan merge a clade twice when nearest neighbor distances tie. The result is a malformed tree: a clade ends up as the child of two different parents, the sizes stop adding up, and the root no longer gathers every point.The nearest neighbor chain is only guaranteed to terminate on a mutual pair while the distance along it strictly decreases, and that is also what stops it from ever revisiting a clade it already holds. Ties break the guarantee. The chain ended only when
argminhappened to return the previous entry, so on a tie it could walk past that entry and back onto a clade already on the chain. The duplicate then outlives the merge that consumes its other occurrence and is merged a second time, after it is already dead.The chain now ends whenever the previous entry is a nearest neighbor of the tip, not only when it is the one
argminreturns, which restores the strict decrease. Comparing the distances alone would miss aNaN, which is never equal to itself, so either condition ending the chain is enough. Writing past the end of the chain buffer is guarded too, sinceNx.indexed_putclamps an out of range index rather than raising.Introduced in #355. Measured over 200,000 random fits on integer coordinates, where distances tie constantly: 38 malformed trees before, 0 after. The regression test fails on the current implementation.
linkage: :wardalso raises on the binary backend, and is fixed here too. Merged clades are masked out of the matrix rather than blanked, so their rows keep stale values that ward's update still reads, and its Lance-Williams term can come out negative for a dead column. Those columns are never used, but the square root of a negative raises where EXLA gives a silent NaN, so the term is clamped at zero. Also introduced in #355: 17 crashes in 600 random fits before, 0 after.The chain guard also covers a third case from #355: an infinite coordinate puts both infinity and NaN in the distance matrix, the chain then extends without ever finding a mutual pair, and writing past the end of its buffer raised
index 4 is out of boundson every linkage. It now reports the merges it could not make, as the moduledoc describes.