Fix Hierarchical hanging on non-finite dissimilarities - #353
Conversation
|
Tracking which ones is alive requires additional work though, no? I wonder if instead of making it work we could abort? For example, if the thing argmin points to is non-finite, we abort the loop? I also assume the :tie_break option in argmin won't help us... |
Might be overengineering this, so maybe simplify is better. And switching argmin's tie_break to :high doesn't help, the same stuck pair just moves I agree aborting is the better fix. Tracking alive only exists to force a merge when we're stuck, and that merge doesn't actually mean anything (the two clades are stuck precisely because they're both non-finite from each other or from everything else). Better to stop the loop and raise than hand back a dendrogram whose top merge silently carries a meaningless height. @josevalim I'll rework the PR to abort and raise instead, and drop the alive tracking entirely. |
A non-finite dissimilarity, from NaN or infinite values in the data or in a precomputed matrix, can make argmin's tie-breaking point two clades at each other without either being picked as the other's mutual nearest neighbor. No merge happens that round, and since nothing about the state changes, no merge ever happens again: fit/2 loops forever. This cannot happen for finite dissimilarities, where the globally closest pair of live clades is always mutually nearest, guaranteeing at least one merge per round. Stop the loop when a round makes no progress instead of running forever. The merges that could not be made are reported as clades of [-1, -1], sizes of 0, and NaN dissimilarities sorted to the end, rather than guessing a pairing that has no finite distance to justify it. Finite input is unaffected, since a round with no progress is impossible there.
da0285f to
6bf7322
Compare
|
Pushed, with one change from what I said: no raise, because it can't work here.
So the loop still aborts as agreed, but the merges it couldn't make are reported instead: clades of |
Drop the Task timeout helper, since ExUnit already times out a hanging test. Drop the all-NaN case, which exercises the same path as the NaN coordinate one, and the jit_apply case, since jit is already covered elsewhere in the file. Drop the finite data case, which compared fit/2 against itself once the fallback path was removed.
|
💚 💙 💜 💛 ❤️ |
Bug
With NaN or infinity in the dissimilarity matrix,
fit/2can hang forever.Root cause: once only two clades remain and their distance is infinite (or NaN), argmin's
tie-breaking points them at each other asymmetrically (one points to itself, the other to
it), so neither is ever picked as the other's mutual nearest neighbor. No merge happens and
the loop that drives the algorithm never terminates.
This is otherwise impossible for finite input: the globally closest pair of clades is
always mutually nearest to each other, guaranteeing at least one merge every round. Caught
while working on HDBSCAN, which can produce infinite mutual reachability values.
Fix
Track which clades are still alive. If a round makes zero progress, force a merge between
the two lowest-indexed ones still alive. This never triggers on finite input, so it's a
no-op there.
Testing
4 new regression tests, each wrapped in a
Taskwith an explicit timeout so a futureregression fails fast instead of hanging the suite: infinite distance between the last two
clades, a NaN coordinate, an all-NaN input, and a check that results on finite data are
byte-for-byte unchanged across every linkage. Full suite passes, 0 failures.