distance_key is silently exchanged in define_clusters

[dansteiert]

Report

I had trouble assiging the distance key in define_clonotypes, while digging for the issue I identified the issue (me!).

Although I still like to propose a change and can generate a pull request for it if one of my or another suggestion sounds good to you!

by defenition define_clonotypes, does not forward the distance_key to define_clonotype_clusters and thus does not allow for custom distance_keys here - as this is what define_clonotype_clusters does.
Setting distance_key is still possible but is silently redefined in define_clonotypes, which I would like to change.

suggestions:
a) send a log info if distance_key != "ir_dist_nt_identity" that this will be used instead.
b) drop distance_key from the input parameters.
c) both of the above (if passed as a kwargs)

Versions

[grst]

Thanks for letting me know! It certainly doesn't make sense to have distance_key in the function signature and then ignore it. entirely.

I'd be inclined to remove it entirely, and raise an error if specified. After all, as you said, define_clonotypes should always use identity distance and for everything else, define_clonotype_clusters should be used.

An alternative would be to actually pass the variable on to define_clonotype_clusters.

WDYT?

[dansteiert]

I agree - removing the distance key and raising at least a warning or send an error if distance_key is given in the kwargs would be nice.

passing it on regardless defeats the purpose of this function, no? then it does exactly what define_clonotype_clusters does with maybe a bit less flexibility.

[grst]

agreed! Do you want to prepare a PR for the first option?