repair --merge-axiom-annotations is removing axioms it shouldn’t remove #1239
Comments
When preparing import modules, we do a few things: (1) add a dc:source ontology annotation, derived from the version IRI of the original ontology; (2) remove all other ontology annotations, keeping only the newly added dc:source; (3) inject proper SubAnnotationPropertyOf axioms for properties representing subsets and synonym types. All those steps are currently performed by SPARQL queries. Here we replace those queries by calls to the `odk:annotate` command, which takes care of (1) and (2), and to the `odk:normalize` command, which takes care of (3). Of note, the fact that we are no longer going through a SPARQL processing step means that we could end up with duplicated axioms with different sets of annotations. Those were automatically merged as a side-effect of the SPARQL processing (which involves dumping the output of the SPARQL processing and re-parsing it again into OWLAPI objects). Since we no longer benefit from that side-effect, we must explicitly include a step in which we merge duplicated axioms (theoretically this could be done with `robot repair --merge-axiom-annotations`, but unfortunately this command does not behave exactly like we would [1]). [1] ontodev/robot#1239
|
This is such a grossly wrong behaviour (both cases you mention). i cannot even begin to understand how I let that pass through! Here is the merge method: Merge axiom annotations is such a well defined concept; its easily clear what should happen (in both case the axiom and all annotations should be preserved). Can you spot the issue in the code? I can fix, or you can make a PR (I am happy to as well as it is my mess); Thx |
Not entirely. There is one obvious problem: the code is only ever looking at annotated axioms. So by design, it will necessarily miss the case where you have one unannotated axiom and one (or more) logically identical annotated axiom(s). But right now I don’t get how that code would lead to the dropping of the For what it’s worth, my own implementation in my ROBOT plugin is as follows: private void mergeAxioms(OWLOntology ontology) {
OWLOntologyManager mgr = ontology.getOWLOntologyManager();
Set<OWLAxiom> origAxioms = ontology.getAxioms(Imports.EXCLUDED);
Map<OWLAxiom, Set<OWLAnnotation>> annotsMap = new HashMap<>();
for ( OWLAxiom ax : origAxioms ) {
annotsMap.computeIfAbsent(ax.getAxiomWithoutAnnotations(), k -> new HashSet<>())
.addAll(ax.getAnnotations());
}
Set<OWLAxiom> mergedAxioms = new HashSet<>();
for ( Map.Entry<OWLAxiom, Set<OWLAnnotation>> entry : annotsMap.entrySet() ) {
mergedAxioms.add(entry.getKey().getAnnotatedAxiom(entry.getValue()));
}
mgr.removeAxioms(ontology, origAxioms);
mgr.addAxioms(ontology, mergedAxioms);
}In my tests, it is working as I expect. |
I believe you (@matentzn) have fixed that already (c4e9708), but the fix has not yet found its way into a release (this was after the October 1.9.7 release). But there is still the issue that the code is only considering annotated axioms, and therefore does not merge one unannotated axiom with an identical (modulo the annotations) annotated axiom. Based on this comment in the code: // OWLAPI should already merge non-annotated duplicatesthe author assumed that this case was already handled by the OWLAPI. But that assumption is wrong, as illustrated by the GO:0005319 class in my example: the OWLAPI did not automatically merge the non-annotated SubClassOf axiom with its annotated counterpart. |
|
@gouttegd I forgot how much nicer Java code looks then python.. I love your implementation; would you be willing to PR it in this repo? I can test/review it. This should take care of the non-annotated issue as well, right? |
You mean, completing replacing the original implementation by mine? Sure, I can do that.
Yes. |
|
Thanks! |
When preparing import modules, we do a few things: (1) add a dc:source ontology annotation, derived from the version IRI of the original ontology; (2) remove all other ontology annotations, keeping only the newly added dc:source; (3) inject proper SubAnnotationPropertyOf axioms for properties representing subsets and synonym types. All those steps are currently performed by SPARQL queries. Here we replace those queries by calls to the `odk:annotate` command, which takes care of (1) and (2), and to the `odk:normalize` command, which takes care of (3). Of note, the fact that we are no longer going through a SPARQL processing step means that we could end up with duplicated axioms with different sets of annotations. Those were automatically merged as a side-effect of the SPARQL processing (which involves dumping the output of the SPARQL processing and re-parsing it again into OWLAPI objects). Since we no longer benefit from that side-effect, we must explicitly include a step in which we merge duplicated axioms (theoretically this could be done with `robot repair --merge-axiom-annotations`, but unfortunately this command does not behave exactly like we would [1]). [1] ontodev/robot#1239
|
Fixed with #1240. |
I believe there are two (maybe related) problems with the
repair --merge-axiom-annotationsfeature.They can both be illustrated with the following minimal example:
Running
robot repair -i minimal.ofn --merge-axiom-annotations true -o repaired.ofnyieldsThe first problem is with the RO:0001025 property: note how the following axiom
is completely removed.
The second problem is with the GO:0005319 class. The original file contains the same axiom (GO:0005319 SubClassOf BFO:0000050 some GO:0006869) twice: one unannotated, one carrying a
GO_REF:0000090cross-reference annotation. After “repair”, the annotated axiom is completely removed, meaning the cross-reference annotation is lost. This is not the behaviour I would expect for--merge-axiom-annotations-- I would have the annotation to be preserved, and the unannotated axiom to be removed.The text was updated successfully, but these errors were encountered: