EVA-4150: Finish adding ClinVar Xrefs#506
Conversation
tcezard
left a comment
tcezard
left a comment
There was a problem hiding this comment.
Changes looks great but I am a bit confused by the __lt__ and might need a bit of hand hold through it.
| # TODO consider flipping the order of the enums or the mappings so they're more consistent | ||
| return (other.distance, other.get_mapping_source()) < (self.distance, self.get_mapping_source()) | ||
| if isinstance(other, OxoMapping): | ||
| return super().__lt__(other) and self.distance < other.distance |
There was a problem hiding this comment.
Do I understand correctly that for two OxoMapping one is better than the other if it has a strictly better source mapping AND the distance is lower ?
What happens when the two are equal?
There was a problem hiding this comment.
Fixed this (and the similar issue for ZoomaMapping) and added tests... it's not so important since in practice we only use distance=1, but you're right it wasn't implemented correctly before.
Co-authored-by: Timothee Cezard <tcezard@ebi.ac.uk>
|
The less-than function is definitely not intuitive and I'm not sure it's implemented in the best way, so happy to go through it together. What I did was basically come up with some rankings that made sense to me (see here and also the unit tests), and implement the less-than function to replicate those rankings. |
2 participants
See a small test spreadsheet here