-
Notifications
You must be signed in to change notification settings - Fork 53
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
Update OMIM gene references in Mondo #8108
base: master
Are you sure you want to change the base?
Conversation
Aim to have this in the October release if possible. |
I have reviewed this PR carefully and have found the following issues. All of these are pretty major, and she be resolved before we can go ahead with this PR.
|
I am working through these issues. |
Related to point 3. "Genes should not be added if the OMIM record is associated with multiple genes", in tracking back the OMIM processing steps I do not see all the genes in one of the initial files, ie omim.ttl. Content from this file is further transformed and eventually used in the omim pipeline. I think this is a bug in that processing and submitted an issue in our OMIM repo about this. I am mentioning this here, in case others not subscribed for updates in that repo. |
$(MAKE) $(TMPDIR)/external/processed-mondo-omim-genes.robot.owl -B | ||
# We need to be less aggressive here, as some gene relations were not originally sourced | ||
# from OMIM, and were added, for example, for ClinGen. | ||
grep -vE '^(relationship: has_material_basis_in_germline_mutation_in .*source="OMIM:)' $(SRC) > $(TMPDIR)/mondo-edit.tmp || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, only OMIM is used as the source for has_material_basis_in_germline_mutation_in
. However, this update should only remove the axioms where OMIM is the source in case there are different sources in the future.
cc: @matentzn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just note that at least once we should clear out all these axioms, to not ignore some that right now dont have omim as the source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matentzn I do not understand your comment. From what I saw all subClassOf relationships that use has_material_basis_in_germline_mutation_in
have OMIM as the source unless I missed something in looking at that data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comment makes zero sense given:
only OMIM is used as the source for has_material_basis_in_germline_mutation_in
it would make sense if:
- there are has_material_basis_in_germline_mutation_in axioms which have no evidence
- or evidence other than omim
as I think your statement would then not delete them; and it should!
I will add this in draft mode, because this needs extremely careful review by at least @twhetzel and @sabrinatoro.
You can rerun the pipeline by checking out the branch and running
I dont know how you want to review this, but I will remind you:
I am sure there is much that needs to be fixed, but I wanted to get the ball rolling at least.
I cant work more on this, but I think its worth reviewing it and identifying issues.