-
Notifications
You must be signed in to change notification settings - Fork 33
Issue 3027 #3516
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
base: master
Are you sure you want to change the base?
Issue 3027 #3516
Conversation
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.
Changes detected in src/ontology/uberon-edit.obo involving intersection_of. Review by specific Uberon Core Team member is required.
gouttegd
left a comment
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.
It looks like you inadvertently created two “facial crest” terms.
And the first one has logical axioms that can’t be represented in OBO.
src/ontology/uberon-edit.obo
Outdated
| property_value: foaf-homepage "http://uberon.org" xsd:anyURI | ||
| property_value: has_ontology_root_term UBERON:0000104 | ||
| property_value: has_ontology_root_term UBERON:0001062 | ||
| owl-axioms: Prefix(owl:=<http://www.w3.org/2002/07/owl#>)\nPrefix(rdf:=<http://www.w3.org/1999/02/22-rdf-syntax-ns#>)\nPrefix(xml:=<http://www.w3.org/XML/1998/namespace>)\nPrefix(xsd:=<http://www.w3.org/2001/XMLSchema#>)\nPrefix(rdfs:=<http://www.w3.org/2000/01/rdf-schema#>)\n\n\nOntology(\nDeclaration(Class(<http://purl.obolibrary.org/obo/UBERON_0001474>))\nDeclaration(Class(<http://purl.obolibrary.org/obo/UBERON_0007681>))\nDeclaration(Class(<http://purl.obolibrary.org/obo/UBERON_4200133>))\nDeclaration(Class(<http://purl.obolibrary.org/obo/UBERON_4200230>))\nDeclaration(Class(<http://purl.obolibrary.org/obo/UBERON_7500131>))\nDeclaration(Class(<http://purl.obolibrary.org/obo/UBERON_8500006>))\nDeclaration(ObjectProperty(<http://purl.obolibrary.org/obo/BFO_0000050>))\n\n############################\n# Classes\n############################\n\n# Class: <http://purl.obolibrary.org/obo/UBERON_7500131> (<http://purl.obolibrary.org/obo/UBERON_7500131>)\n\nSubClassOf(<http://purl.obolibrary.org/obo/UBERON_7500131> ObjectIntersectionOf(<http://purl.obolibrary.org/obo/UBERON_4200133> ObjectSomeValuesFrom(<http://purl.obolibrary.org/obo/BFO_0000050> <http://purl.obolibrary.org/obo/UBERON_0007681>)))\n\n# Class: <http://purl.obolibrary.org/obo/UBERON_8500006> (<http://purl.obolibrary.org/obo/UBERON_8500006>)\n\nSubClassOf(<http://purl.obolibrary.org/obo/UBERON_8500006> ObjectIntersectionOf(<http://purl.obolibrary.org/obo/UBERON_4200230> ObjectSomeValuesFrom(<http://purl.obolibrary.org/obo/BFO_0000050> <http://purl.obolibrary.org/obo/UBERON_0001474>)))\n\n\n) |
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 are trying to insert the following axiom:
'facial crest' SubClassOf: 'crest' and ('part of' some 'facial neural crest')
Such an axiom cannot be represented in “pure” OBO format, hence the introduction of this owl-axioms tag.
If you intended to say a 'facial crest' is both a 'crest' and something that is 'part of' some 'facial neural crest', you should do so in two independent SubClassOf axioms, there is no need for an ObjectIntersectionOf.
If you intended to say that any 'crest' that is 'part of' some 'facial neural crest' is a 'facial crest', then what you need is a EquivalentTo axiom, not a SubClassOf axiom. Such an axiom is representable in “pure” OBO.
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.
@gouttegd do you have any guidance for when EquivalentTo should be used ('crest' and 'part of' some 'facial neural crest') or SubClaseOf ('crest', 'part of' some 'facial neural crest')? The facial crest is not a crest can pop up anywhere (as seems to be facial neural crest), it is specifically on the lateral side between the maxilla and zygomatic arch.
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.
There are some pages on the OBOOK about the issue (for example here and here), though it’s nowhere near as complete as it should be IMHO.
But the most important rule of thumb, I believe, is: When in doubt, use SubClassOf. :) Whenever you are unsure which of SubClassOf or EquivalentTo is more appropriate, it can never be wrong to use SubClassOf (the worst that can happen is that you will make a statement that won’t be as “strong” as it should be, but that will be correct all the same).
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.
However, independently of the choice between SubClassOf and EquivalentTo, there may also be a problem with the term involved: are you sure this 'facial crest' is 'part of' some 'facial neural crest'?
'facial neural crest' is a part of the neural crest, an embryonic structure. That does not seem to fit well with the text definition.
Of note, the original term requested had proposed a logical definition involving 'facial skeleton', not 'facial neural crest'.
src/ontology/uberon-edit.obo
Outdated
|
|
||
| [Term] | ||
| id: UBERON:8500008 | ||
| name: DEPRECATED Facial crest |
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.
Given that the term has never made it into a public release (it did not even make it into the master branch!), it would have been perfectly acceptable to simply delete it without leaving a trace.
|
@meghalithic There are a lot of minor conflicts, likely due to the fact that the first commit of the PR was done nearly two years ago. Do you want me to resolve them for you? |
|
@gouttegd yes please! Thank you! |
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.
Changes detected in src/ontology/uberon-edit.obo involving intersection_of. Review by specific Uberon Core Team member is required.
gouttegd
left a comment
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.
The conflicts have been fixed (which makes the changes much easier to review now that they are no longer interspersed with unrelated changes).
The logical definition of 'nasal notch' is problematic, see below.
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.
Changes detected in src/ontology/uberon-edit.obo involving intersection_of. Review by specific Uberon Core Team member is required.
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.
Changes detected in src/ontology/uberon-edit.obo involving intersection_of. Review by specific Uberon Core Team member is required.
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.
Changes detected in src/ontology/uberon-edit.obo involving intersection_of. Review by specific Uberon Core Team member is required.
|
This PR has not seen any activity in the past month; if nobody comments or reviews it in the next week, the PR creator will be allowed to proceed with merging without explicit approval, should they wish to do so. |
issue #3027
creating the following terms: facial crest, nasal notch, and staphylon