-
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
Rename OMIM axiom annotation properties #698
base: develop
Are you sure you want to change the base?
Conversation
- Update OMIM properties in properties.txt to the correct spelling / casing.
@@ -15,8 +15,8 @@ http://www.geneontology.org/formats/oboInOwl#source | |||
http://www.geneontology.org/formats/oboInOwl#hasSynonymType | |||
http://www.geneontology.org/formats/oboInOwl#SynonymTypeProperty | |||
http://purl.obolibrary.org/obo/mondo#GENERATED | |||
http://purl.obolibrary.org/obo/mondo#omim_included | |||
http://purl.obolibrary.org/obo/mondo#omim_formerly | |||
http://purl.obolibrary.org/obo/mondo#includedEntryInOMIM |
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.
Don't these need to actually appear in properties.txt
?
During the tech meeting today, @matentzn was surprised I was calling them properties. But I feel like they belong here, alongside http://purl.obolibrary.org/obo/mondo#GENERATED
which I think was disappearing if we didn't have it showing in this file.
I could do a before/after test to check this myself but for now I'll just pose the question.
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.
I dont think this needs to be here. Most of the things you see here, like "GENERATED" are synonym type properties.. That is very different from the MONDO:includedEntryInOMIM
source annotations (see Mondo file)
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.
Aren't the also supposed to be synonym type properties? Is there anything about the nature of MONDO:GENERATED
that makes it more appropriate to say that it is a property of the synonym's type than the OMIM ones?
I remember that we have been discussing making object property declarations (I believe for these) in omim.ttl
, but have not yet.
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.
At some point things were failing without these being added, but this may not be the location that fixed things.
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.
Look at how includedInOMIM
is used in Mondo:
xref: OMIM:184850 {source="MONDO:includedEntryInOMIM"}
As a oboInOwl:source
annotation. This is not at all like:
synonym: "undetermined early-onset epileptic encephalopathy" EXACT GENERATED []
Which is how the GENERATED synonym type was used.
The confusion of why includedEntryInOMIM
isn't used the same way, i.e.
synonym: "undetermined early-onset epileptic encephalopathy" EXACT includedEntryInOMIM []
That question is valid! I don't know the exact answer, but lets say for the sake of rapid progress now that the reason is historical.
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.
Yea maybe.. I dont think you really need the synonym type "omim included", but you may need the synonym type "omim formerly". Right now I think we should do whatever can be done right now and open issues to revise the decision if need be..
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.
@joeflack4 have you looked further into this? Also, I set this to merge into develop
since it was set incorrectly.
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.
I had this on our recent 1:1 agenda, section titled "MONDO: omimFormerly & includedEntryInOMIM".
Can wait until our next meeting if you like.
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.
@joeflack4 can you sort out if this change is needed, try to run relevant make goals with and without the change to check and any other ways you may think of to test this.
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.
@twhetzel May want to just move this to our meeting, that might be easier. This is basically a sub issue of:
And the answers to those questions / decisions will impact whether this is needed or not. If they will become synonym types as Nico seems to suggest might be appropriate, these will definitely be needed.If you / we decide that they should not be changed to that, then I can go ahead and run such an examination now.
@@ -15,8 +15,8 @@ http://www.geneontology.org/formats/oboInOwl#source | |||
http://www.geneontology.org/formats/oboInOwl#hasSynonymType | |||
http://www.geneontology.org/formats/oboInOwl#SynonymTypeProperty | |||
http://purl.obolibrary.org/obo/mondo#GENERATED | |||
http://purl.obolibrary.org/obo/mondo#omim_included | |||
http://purl.obolibrary.org/obo/mondo#omim_formerly | |||
http://purl.obolibrary.org/obo/mondo#includedEntryInOMIM |
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.
Spoke with Nico today: conclusions.
-
@joeflack4 KeepincludedEntryInOMIM
- @joeflack4 Also keep
omimIncluded
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.
No, includedEntryInOMIM has nothing to do here; it is not a property. It is a source code value. Yes. It should be kept in mondo. No, it should not be kept in this list.
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.
Yes you're right. Not sure why I put that checkbox on this particular PR. But it does also need:
- @joeflack4
omimFormerly
Warning
These should be merged at the same time:
included
/formerly
refactor omim#167Overview
Update OMIM properties in properties.txt to the correct spelling / casing.
Context
This is a follow-up from our tech meeting today. We discussed:
MONDO:omim_formerly
mondo#8235I tried to show that the casing (snake vs camel) wasn't consistent between repos but I couldn't find an example.
Basically in
mondo-ingest
andomim
repos, it's snake case. But now inmondo
, it's camel case (monarch-initiative/mondo#8235, axiom annotation docs).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?