search

theislab / scib

Benchmarking analysis of data integration tools
MIT License
294 stars 63 forks source link

Negative cLISi score #282

Closed Hrovatin closed 2 years ago

Hrovatin commented 2 years ago

We got negative cLISi score. This could happen if nBatches<lisi. Any ideas why this could be the case in our data (we had recomputed neighbours before running it). @soroorh

LuckyMD commented 2 years ago

Hey! We've seen this occasionally as well, but it was typically not reproducible and seemed to be due to a numerical issue. Do you have a reproducible example? Also, what do you mean by "lisi" in your nBatches<lisi?

Hrovatin commented 2 years ago

I was referring to lisi in https://github.com/theislab/scib/blob/172f181b3e0e6118e956510725755f9a7c1aeab0/scib/metrics/lisi.py#L700

@soroorh Could you try to rerun this metric on the specific dataset to see if it happens again?

soroorh commented 2 years ago

Done. We still get a negative value, i'm afraid. In this case, there are only 2 batches, so my impression is that the numerator is negative.

Hrovatin commented 2 years ago

@LuckyMD any ideas?

LuckyMD commented 2 years ago

Ah... could it be that you have n_cell_types > n_batches? I think this is a bug from treating ilisi and clisi together the whole time. This should be (n_cell_types - clisi_score) / (n_batches - 1) to have the correct upper limit.

LuckyMD commented 2 years ago

The only thing that needs to be adapted is: https://github.com/theislab/scib/blob/172f181b3e0e6118e956510725755f9a7c1aeab0/scib/metrics/lisi.py#L300

batch_key -> label_key

The part you reference above is deprecated anyway

LuckyMD commented 2 years ago

Hey you guys! You can pull from #287 if you want. The test pipeline values still need to be adapted for this to be merged. That's sth @mumichae set up, so I would prefer to wait for her to adapt those. But technically this should fix it.

soroorh commented 2 years ago

all good. confirm it's now fixed. thanks heaps, Malte!

LuckyMD commented 2 years ago

Adapted testing and merged now as well. You can use master branch then. Thanks for bringing this up!