-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove unneeded HGNC SPARQL updates #459
Conversation
- Update: In makefile, in components goals, removed unnecessary robot query updates involving HGNC mappings. This query only applies to OMIM.
@@ -113,7 +113,6 @@ $(COMPONENTSDIR)/doid.owl: $(TMPDIR)/doid_relevant_signature.txt | component-dow | |||
remove -T $(TMPDIR)/doid_relevant_signature.txt --select complement --select "classes individuals" --trim false \ | |||
query \ | |||
--update ../sparql/fix_omimps.ru \ | |||
--update ../sparql/fix_hgnc_mappings.ru \ |
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.
File Rename File & ref deletion
If indeed I'm correct and fix_hgnc_mappings.ru
only applies to OMIM, why don't we prepend omim_
to the file name for disambiguation?
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.
You could, but even better: is this "fix" still necessary? Maybe it can be removed altogether?
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.
That's a good question. I'll look into that.
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.
Is this still an open action item, to look into whether the "fix" hgnc mappings is still necessary?
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.
yep
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 You were correct. I ran with and without this SPARQL update, and there was no difference. I've deleted the file and its references. Feel free to merge!
- Delete: fix_hgnc_mappings.ru and references. It is no longer needed.
Thank you for the great simplification and refactoring! |
Overview
In makefile, in components goals, removed unnecessary robot query updates involving HGNC mappings. This query only applies to OMIM. This does not affect outputs, just code cleanliness.
Additional details
I'm not sure how these lines got there in the first place. In the ICD10WHO instance, I'm pretty sure it was me copy/pasting over the lines, but I think someone else might have added them erroneously in the other cases, maybe.
Pre-merge checklist
Documentation
Was the documentation added/updated under
docs/
?QC
Was the full pipeline run before submitting this PR using
sh run.sh make build-mondo-ingest
on this branch (afterdocker pull obolibrary/odkfull:dev
), and no errors occurred?New Packages
Were any new Python packages added?
Were any other non-Python packages added?
PR Review and Conversations Resolved
Has the PR been sufficiently reviewed by at least 1 team member of the Mondo Technical team and all threads resolved?