Bug in calculating all_by_all_pairwise_similarity

[hrshdhgd]

When len(ancestor_set) == 0 the jacquard and AIC scores are explicitly set to 0 which is incorrect.

Thank you @souzadevinicius for highlighting this!

Also updated semsimian version dependency to latest

fixes monarch-initiative/semsimian#133 (comment)

[caufieldjh]

OK, so what was happening was that the provided nodes didn't have ancestors (at least not in the provided set), so their metrics were set to zero. Should this produce an error and/or should there be an attempt (in oaklib or semsimian) to identify the potential ancestor? It's reasonable to expect that the user should only need to provide IC values for nodes of interest, even if the ancestors are required to calculate pairwise semsim.

[justaddcoffee]

Could the 3 of us do a code review on this, maybe tomorrow if we are all around?

OK, so what was happening was that the provided nodes didn't have ancestors (at least not in the provided set), so their metrics were set to zero.

If I understand the code correctly, we check here if the two terms have a common ancestor (which we call ancestor_set, but I think is a bad name - it's a tuple of ancestor_id and the IC value of this ancestor). I think the correct behavior is what we do here, which is to set the common ancestor ID in the similarity object if we have it, otherwise set it to OWL_THING or some such.

I think the remaining question is why so many (all?) of Vinicius's pairs of ontology terms didn't have a common ancestor? This seems like a bug somewhere, either in OAK, semsimian, or Phenio.

[caufieldjh]

I think the remaining question is why so many (all?) of Vinicius's pairs of ontology terms didn't have a common ancestor? This seems like a bug somewhere, either in OAK, semsimian, or Phenio.

They certainly have common ancestors, and in PHENIO the common ancestor of an HP term and an MP term will almost always be a UPHENO term. But somehow if the UPHENO term isn't included in the provided IC map it doesn't get included as an ancestor, even if it's present in the source ontology - perhaps because we don't mix user-provided IC maps with calculated ones.