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

OBOgraphs fails hard in the presence of complex property expression #93

Closed
matentzn opened this issue Feb 19, 2023 · 10 comments
Closed

Comments

@matentzn
Copy link

Minimal test:

<?xml version="1.0"?>
<rdf:RDF xmlns="http://purl.obolibrary.org/obo/fail-expression.owl#"
     xml:base="http://purl.obolibrary.org/obo/fail-expression.owl"
     xmlns:owl="http://www.w3.org/2002/07/owl#"
     xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
     xmlns:xml="http://www.w3.org/XML/1998/namespace"
     xmlns:xsd="http://www.w3.org/2001/XMLSchema#"
     xmlns:rdfs="http://www.w3.org/2000/01/rdf-schema#">
    <owl:Ontology rdf:about="http://purl.obolibrary.org/obo/fail-expression.owl">
    </owl:Ontology>
    
    <owl:Class rdf:about="http://purl.obolibrary.org/obo/OBI_0000260">
        
        <rdfs:subClassOf>
            <owl:Restriction>
                <owl:onProperty>
                    <rdf:Description>
                        <owl:inverseOf rdf:resource="http://purl.obolibrary.org/obo/COB_0000087"/>
                    </rdf:Description>
                </owl:onProperty>
                <owl:allValuesFrom rdf:resource="http://purl.obolibrary.org/obo/COB_0000082"/>
            </owl:Restriction>
        </rdfs:subClassOf>
    </owl:Class>
   
</rdf:RDF>

This fails because of:

<owl:onProperty>
    <rdf:Description>
        <owl:inverseOf rdf:resource="http://purl.obolibrary.org/obo/COB_0000087"/>
    </rdf:Description>
</owl:onProperty>

The rare case where we use property expressions instead of properties.

@matentzn
Copy link
Author

@julesjacobsen can you take care of this?

@julesjacobsen
Copy link
Collaborator

What is the expected behaviour for the test to pass?

@matentzn
Copy link
Author

Imo the unsupported construct should be ignored (logger.warning) and serialisation resumed without it. (Non draconian error handling)

@julesjacobsen
Copy link
Collaborator

OK, but instead of ignoring it in the output and logging an error, would this make sense:

---
graphs:
- id: "http://purl.obolibrary.org/obo/fail-expression.owl"
  domainRangeAxioms:
  - predicateId: "http://purl.obolibrary.org/obo/COB_0000087"
    allValuesFromEdges:
    - sub: "http://purl.obolibrary.org/obo/OBI_0000260"
      pred: "inverseOf"
      obj: "http://purl.obolibrary.org/obo/COB_0000087"

@cmungall
Copy link
Member

no, this would translate to

OBI_0000260 subClassOf inverseOf only COB_0000087

This should fall into a general bucket where we can provide a generic translation, see #94

@julesjacobsen
Copy link
Collaborator

ahh, so these go into the untranslatedAxioms list in the FromOwl class? These are only ever collected and never used afterwards.

In this case the untranslated axioms would be:

Untranslated axioms: [SubClassOf(<http://purl.obolibrary.org/obo/OBI_0000260> ObjectAllValuesFrom(ObjectInverseOf(<http://purl.obolibrary.org/obo/COB_0000087>) <http://purl.obolibrary.org/obo/COB_0000082>))]

There are a lot of guards to check that the entity is named (here is one of them), so should anything failing this be added to the untranslatedAxioms list?

e.g.

else if (supc instanceof OWLObjectAllValuesFrom) {
    OWLObjectAllValuesFrom avf = (OWLObjectAllValuesFrom) supc;
    OWLObjectPropertyExpression property = avf.getProperty();
    if (property instanceof OWLObjectProperty) {
        DomainRangeAxiom.Builder b = getDRBuilder(property, domainRangeBuilderMap);
        if (avf.getFiller().isNamed()) {
            Edge e = getEdge(subj,
                    //TODO CHECK!!!
                    b.build().getPredicateId(),
                    getClassId(avf.getFiller().asOWLClass()),
                    nullIfEmpty(meta));
            b.addAllValuesFromEdge(e);
        } else {
// this wasn't present before
            untranslatedAxioms.add(sca);
        }
    } else if (property instanceof OWLObjectInverseOf) {
// This is new but might be incorrect  OWLObjectAllValuesFrom can either be a OWLObjectProperty or a OWLObjectInverseOf. 
// OWLObjectInverseOf wasn't handled before which is leading to the error
        OWLObjectInverseOf iop = (OWLObjectInverseOf) property;
        if (avf.getFiller().isNamed() && iop.isNamed()) {
            String pid = getPropertyId(iop.getInverse().asOWLObjectProperty());
            DomainRangeAxiom.Builder b = getDRBuilder(iop.getInverse(), domainRangeBuilderMap);
            Edge edge = getEdge(subj, INVERSE_OF, pid, meta);
            b.addAllValuesFromEdge(edge);
        } else {
            // this wasn't present before
           untranslatedAxioms.add(sca);
        }
    }
}

@matentzn
Copy link
Author

Needs @cmungall but && iop.isNamed() is exactly what is needed I think; not sure about the avf.getFiller().isNamed() but I guess this is one of the current obographs limitations, so yeah, looks good. Can you make a PR? I will get chris to review this.

@julesjacobsen
Copy link
Collaborator

At present, in my working branch, the output is like so

---
graphs:
- id: "http://purl.obolibrary.org/obo/fail-expression.owl"
  nodes:
  - id: "http://purl.obolibrary.org/obo/OBI_0000260"
    type: "CLASS"

with the following being logged to the console

12:26:54.724 [main] WARN  o.g.obographs.owlapi.FromOwl - http://purl.obolibrary.org/obo/fail-expression.owl contains 1 Untranslated axioms:
12:26:54.732 [main] WARN  o.g.obographs.owlapi.FromOwl - SubClassOf(<http://purl.obolibrary.org/obo/OBI_0000260> ObjectAllValuesFrom(ObjectInverseOf(<http://purl.obolibrary.org/obo/COB_0000087>) <http://purl.obolibrary.org/obo/COB_0000082>))

@matentzn , @cmungall does this seem acceptable?

@matentzn
Copy link
Author

matentzn commented May 3, 2023

I love it!

julesjacobsen added a commit to julesjacobsen/obographs that referenced this issue May 3, 2023
Change for geneontology#90 - ensure all declared nodes (CLASS, INDIVIDUAL, PROPERTY) are added even if unlabelled.
Change for geneontology#93 - log untranslated axioms to WARN
Update examples/ with changes
Enable obographs-cli to convert ttl to json and yaml
@julesjacobsen
Copy link
Collaborator

@matentzn @cmungall @balhoff The PR #98 needs approval for this to be closed.

@matentzn matentzn mentioned this issue Oct 26, 2023
6 tasks
julesjacobsen added a commit that referenced this issue Nov 8, 2023
Change for #65 - add PropertyType to PROPERTY Nodes
Change for #90 - ensure all declared nodes (CLASS, INDIVIDUAL, PROPERTY) are added even if unlabelled. 
Change for #93 - log untranslated axioms to WARN
Update examples/ with changes
Update schema/ with changes
Enable obographs-cli to convert ttl to json and yaml
Update for #103 - remove Guava dependency from core
Update Junit to Jupiter 5.0.3
Update  version to 0.3.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants