Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache fix in mondo_score_utils, now faster #27

Merged
merged 9 commits into from
Jun 21, 2024
Merged

Conversation

leokim-l
Copy link
Member

No description provided.

Got rid of profiling tool in runner, forgot to this before

descendants_list = mondo.descendants([prediction], predicates=[IS_A], reflexive=True)
for mondo_descendant in descendants_list:
if ground_truth in omim_mappings(mondo_descendant):
if ground_truth in omim_mappings(mondo_descendant, mondo):
# prediction is a MONDO that maps to a correct OMIM via a descendant
return PARTIAL_SCORE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we should provide full score (full rank) if a disease is in the same phenotypic series, but not necessarily if it is a Mondo descendent. I am not sure if that is what the code above is doing, but if so it would be good to add a comment and an example of what does and does not count (lines 72-74). I am not sure that we want to return a partial score for anything else, and it would really be better just to report ranks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the picture and description at #14 I assumed that all mondo descendants of a mondo term corresponding to a pheontypic series (I mean all descendants of the parent or "grouping" term, for lack of better words -- the one which does not have an OMIM ID on its own) are leaf terms. If this is not the case, @pnrobinson we need to discuss, I am not finding much info about phenotypic series online ...

Also, do note that PARTIAL_SCORE is there in case we were to decide that we want to assign partial scores, which for now we are not doing (both FULL_SCORE and PARTIAL_SCORE are mapped to 1).

Copy link
Member

@pnrobinson pnrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand the software well enough to provide a detailed review, but I did provide some comments about individual lines. Let's discuss.
PS, please also figure out why the CI is not working. I did not understand the readout?

@leokim-l leokim-l mentioned this pull request Jun 20, 2024
@leokim-l
Copy link
Member Author

Fixed CI error, it was a docstring test failing,

a0bd2ee

@leokim-l leokim-l merged commit a2181fd into main Jun 21, 2024
1 check passed
@leokim-l
Copy link
Member Author

Merged since no more conflicts were present, some of the mentioned things that still need to be done are now issues and can be done independently after the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants