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

oboformat: New behavior for oboedit merges, to preserve IRIs #317

Open
cmungall opened this Issue Nov 17, 2014 · 18 comments

Comments

Projects
None yet
5 participants
@cmungall
Copy link
Member

cmungall commented Nov 17, 2014

OE allows merging of a class A into a class B. This destroys metadata about A, but this is typically copied into B.

The result of merging A into B, in .obo:

id: B
alt_id: A

Current translation:

id: B
alt_id: A
<=>
Class: B
Annotations: hasAlternateId "A"  ## "A" is the OBO-style ID

Disadvantage: there is no declaration or reference to the IRI for A

Proposed new translation:

id: B
alt_id: A
<=>
Class: A
Annotations: 
  is_deprecated="true"^^xsd:boolean
  oboInOwl:isMerged="true"^^xsd:boolean
  oboInOwl:replacedBy=B

Note that the new AP isMerged is necessary here for obo2owl roundtripping; replacedBy is not sufficient in itself as this is used for what OE/.obo calls "obsoletion" which is treated distinctly

For backwards compatibility we would retain one-way translation of old pattern:

id: B
alt_id: A
<=   ## THIS DIRECTION ONLY
Class: B
Annotations: hasAlternateId "A"  ## "A" is the OBO-style ID
@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Nov 17, 2014

Please assign this to @hdietze (I don't have permissions). We'll do this on a fork or branch first.

@ignazio1977

This comment has been minimized.

Copy link
Contributor

ignazio1977 commented Nov 18, 2014

I've created a team of OWLAPI contributors and added you and Heiko to it - this should enable you to assign issues.

@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Nov 25, 2014

Still can't assign Heiko (but he knows it's on his plate)

@sesuncedu

This comment has been minimized.

Copy link
Contributor

sesuncedu commented Nov 25, 2014

Did he accept the invitation? Must be on the list to be assigned issues.
https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/

@ignazio1977

This comment has been minimized.

Copy link
Contributor

ignazio1977 commented Nov 25, 2014

Yes he has accepted (don't know exactly when :-) ). I see Chris has assigned the issue to him, so I'm guessing all is working as expected.

@sesuncedu

This comment has been minimized.

Copy link
Contributor

sesuncedu commented Nov 25, 2014

Also, is A a subclass of or equivalent to B? If so, it might be useful to add the appropriate axiom. That axiom could carry the appropriate annotations for the round-trip.

@sesuncedu

This comment has been minimized.

Copy link
Contributor

sesuncedu commented Nov 25, 2014

I think after I sent the message :-)
On Nov 25, 2014 3:39 PM, "Ignazio Palmisano" notifications@github.com
wrote:

Yes he has accepted (don't know exactly when :-) ). I see Chris has
assigned the issue to him, so I'm guessing all is working as expected.


Reply to this email directly or view it on GitHub
#317 (comment).

@alanruttenberg

This comment has been minimized.

Copy link

alanruttenberg commented Nov 26, 2014

@sesuncedu No, we wouldn't make them equivalent - obsolete terms have their axioms removed. For error checking purposes they may even be equated to owl:Nothing, which make it easy to expose use of them.

@alanruttenberg

This comment has been minimized.

Copy link

alanruttenberg commented Nov 26, 2014

@cmungall I'd like you to consider using the ontology-metadata mechanism for obsolescence, which is used by OBI (as well as some others), and doesn't need addition of new properties. In that schema we use the annotation property has_obsolescence_reason (http://purl.obolibrary.org/obo/IAO_0000231) the object of which is an instance of obsolescence reason specification (http://purl.obolibrary.org/obo/IAO_0000225). There is already an instance for merging (http://purl.obolibrary.org/obo/IAO_0000225), but the idea is that one extends the reasons by adding a new instance. In this case the annotation property term replaced by (http://purl.obolibrary.org/obo/IAO_0100001) would then correspond to oboInOwl:replacedBy. The documentation for term merging states that reasons for merging should be recorded (when present) as an editor note. If we want to distinguish the OBO merging case then we would create a new (singleton, as with the rest) instance and document it accordingly.

@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Nov 26, 2014

Apologies, there was an error in my spec above. IAO_0100001 is used, not oboInOwl:replacedBy. Should I edit the original ticket?

I support the addition of any and all other kinds of metadata including has_obsolescence_reason, but there is no cognate of this in the oboformat syntax and no way of adding this in OE so it's out of scope here.

@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Nov 26, 2014

I think we could use IAO_0000225 to denote a merge but it's not completely clear how to do so in a way that's roundtrippable - can you elaborate?

@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Dec 17, 2014

New 'spec':

id: B
alt_id: A
<=>
Class: A
Annotations: 
  owl:is_deprecated="true"^^xsd:boolean
  obo:IAO_0000231=obo:IAO_0000227
  obo:IAO_0100001=B

Where obo is the prefix for http://purl.obolibrary.org/obo

Where

  • IAO_0000231 'has obsolescence reason' (an annotation property)
  • IAO_0000227 'terms merged' (an individual)
  • IAO_0100001'term replaced by' (an annotation property)
@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Mar 3, 2015

what's the status of this?

@hdietze

This comment has been minimized.

Copy link

hdietze commented Mar 10, 2015

It looks like some of the changes made it into the 3.5.2 release.
Unfortunately, the back-port from 4.0.x into 3.5.x seems to break in the Owl2OBO behavior. The test cases was not back moved. The generated deprecated classes or properties are not ignored for the translation into OBO.
The missing back-ported corresponding test case (Owl2OboAltIdTest in contract) will fail.

hdietze pushed a commit that referenced this issue Mar 10, 2015

hdietze
Test case for broken OBO alt_id handling
Back port missing test case for translating alt_ids to obsolete
classes. Seems to be broken in 3.5.2 release. See issue #317
@ignazio1977

This comment has been minimized.

Copy link
Contributor

ignazio1977 commented Mar 11, 2015

Looks like I missed a class. I'll fix that.

ignazio1977 added a commit that referenced this issue Mar 11, 2015

@ignazio1977

This comment has been minimized.

Copy link
Contributor

ignazio1977 commented Apr 7, 2015

As far as I can tell, this is now fixed

@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Jun 14, 2016

This is not a bug as such. More of a problem with underspecification for the original behavior. Noting it here, but not sure any action should be taken.

Consider the following perverse obo

[Term]
id: HP:0000567
name: Chorioretinal coloboma
alt_id: HP:0000611

[Term]
id: HP:0000611
name: This rdfs label axiom is lost in translation
alt_id: HP:0007784
def: "This definition axiom is lost in translation." []
is_obsolete: true

We have two conditions here that should never happen in the wild:

  • an alt_id points to an obsolete class. In terms of the OBO Document structure, the alt_id should always be 'dangling', there is no metadata about the merged class
  • An obsolete class has an alt_id. Only non-obsolete classes should have alt_ids

We in the OBO Document world need to have checkers in place to ensure conditions like the above don't happen (this was always enforced by oboedit, but now we are living in a mixed obo/owl editing world strange things can happen)

When these situations do occur together, then the two axioms on HP:0000611 are lost. The end result is:

[Term]
id: HP:0000567
name: Chorioretinal coloboma
alt_id: HP:0000611

[Term]
id: HP:0000611
alt_id: HP:0007784

What should actually happen is underspecified.

cc @drseb @balhoff @dosumis

@cmungall

This comment has been minimized.

Copy link
Member Author

cmungall commented Jan 19, 2018

There is a second class of perverse problem, simpler than the above one, mentioned in Phenomics/hpo-obo-qc#1

[Term]
id: A
name: a
alt_id: B

[Term]
id: B
name: b
is_obsolete: true

when converted we end up with

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(AnnotationProperty(<http://www.geneontology.org/formats/oboInOwl#id>))\n\n\nAnnotationAssertion(<http://www.geneontology.org/formats/oboInOwl#id> <http://purl.obolibrary.org/obo/bad#B> \"B\"^^xsd:string)\nAnnotationAssertion(rdfs:label <http://purl.obolibrary.org/obo/bad#B> \"b\"^^xsd:string)\n)

[Term]
id: A
name: a
alt_id: B

the root cause is the same as the above: the alt_id should always be dangling. the owlapi accepts the invalid obo but refuses to write all axioms as obo since they don't conform to the obo profile.

we will put checks in downstream in robot but we may want to consider changing the owlapi writer as well to roundtrip this despite the nonconformance

cmungall added a commit to ontodev/robot that referenced this issue Jan 19, 2018

@ignazio1977 ignazio1977 reopened this Jan 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.