scipy linkage called incorrectly

[mriglobal]

I might be wrong here, but it looks like the scipy linkage call invoked for the dendrogram is silently producing misleading output. The function signature for linkage expects either a condensed format distance matrix, or a "2D array of observation vectors" which it then passes to the scipy pdist function to generate a distance matrix. The output from sourmash compare is a similarity matrix which must be converted to a distance matrix via 1 - M and then converted to condensed format by the scipy squareform function. Sourmash invokes the linkage function on the similarity matrix directly where it appears to assume this is the "2D array of observation vectors" case, which it is not. I get very different dendrograms by these two methodologies. Is this working as intended?

[ctb]

hmm, that could indeed be a problem!

We do have a --distance output for sourmash compare so it would be an easy fix.

[mriglobal]

I should point out, that in terms of maintaining meaningful relationships, this doesn't seem to be that impactful. In other words, instances that are similar to one another will also have similar pairwise distance comparisons to other instances.

[ctb]

I should point out, that in terms of maintaining meaningful relationships, this doesn't seem to be that impactful. In other words, instances that are similar to one another will also have similar pairwise distance comparisons to other instances.

that is very good to know!

but it is still concerning!

I'm going to point the clustering mavens at this one and let them work it out - @mr-eyes @bluegenes could you weigh in, pls?

If it turns out to be a situation where we should fix this (as I suspect), I'd also appreciate thoughts on what we should do - should we

• make this a priority for a 4.7.0 release, where we change the behavior of sourmash plot because it's a bug?
• fix this in sourmash v5? v5 which should happen "soon" but not, like, in the next few months. note that we were already thinking of switching the default distance metric to ANI per should we default to ANI based on average containment for compare? #2276 some questions about ANI and compare #2226

ref also detecting oddball matrices per #2228

my hot take based on previous comment that this still maintains relationships is that we should fix it in v5, but it's not a strong opinion.

thanks again for finding this @mriglobal !

[ctb]

doing what I do best: punting to a different repository 😆

Over in the betterplot plugin, I hope to explore this and get it working "properly" - see sourmash-bio/sourmash_plugin_betterplot#15 - and then backport fixes and verification stuff into sourmash compare.