Include SubObjectPropertyOf axioms when reducing.#1248
Include SubObjectPropertyOf axioms when reducing.#1248jamesaoverton merged 4 commits intoontodev:masterfrom
Conversation
When evaluating redundant SubClassOf axioms for the Reduce operation, include SubObjectPropertyOf axioms in the graph, so that if we have SubClassOf(C, P1 some D) SubClassOf(C, P2 some D) SubObjectPropertyOf(P2, P1) then the first SubClassOf axiom can be considered redundant and removed. Also explicitly include SubPropertyChainOf axioms, so that similarly redundant axioms when a property chain is involved can be removed as well.
|
@cmungall Asking for your review since you wrote the original code for the Reduce operation (and you also wrote commit cdbb804, which restricted the types of axioms considered for evaluating redundancy). @jamesaoverton Should the behaviour be configurable by a new command line option to |
matentzn
left a comment
There was a problem hiding this comment.
This change is very consequential - and I am actually a bit surprised that this was not already happening.
I would, however, suggest to add a test for this change, as it has the potential for huge consequences on a lot of OBO (esp. ODK-managed) ontologies..
|
@matentzn I plan to add tests once I have confirmation the feature (which is more of a bug fix in my opinion, as I think |
|
Why not just add a Redundancy is not straightforward, see: Apologies for any confusion caused. When I originally proposed this: I suggested separate commands from removing redundancy (sensu OWL entailment) and graph transitive reduction, we probably should have stuck to that (with graph operations happening in a framework independent of the owlapi / owl reasoning), as these are really different concerns and use cases. But I think I may have overloaded them. |
|
@cmungall I don’t mind making the behaviour configurable by a command-line option, but I defer to @jamesaoverton for the decision. Personally, I think that, if the behaviour is to be made configurable, it should still be enabled by default, with an option to disable it. For two reasons: (1) There is nothing in the description of (2) This would be consistent with the existing options |
There was a problem hiding this comment.
I think it sounds good to have this feature, but it should be added as a non-default CLI option. We have been relying on the previous behavior, at least in GO, to materialize more standard relations from highly specific ones (e.g. 'part of' from 'bounding layer of') using a combination of materialize and reduce. My understanding is that this would stop working after this change.
Do not unconditionally take into account SubObjectPropertyOf axioms when evaluating redundancy in the context of a Reduce operation. Only do so when this is expressly asked for with a new command-line option, to avoid breaking existing pipelines. (For the record, I believe this should be enabled by default, and that "existing pipelines" could use an option to disable it instead.)
Document the new --include-subproperties option to `reduce`. Show an example command, which also (as usual throughout the ROBOT documentation) acts as a test case.
|
@balhoff Option tentatively added. :) I defer to @jamesaoverton for the decision as to whether it should be enabled or disabled by default (for now it’s disabled, it’s one line to change in |
|
I agree with @balhoff: we should stick to the old behaviour by default, to maintain backwards compatibility, even if we agree that the old behaviour was buggy. (Sorry, but backwards compatibility is very important to the ROBOT project.) Thanks very much! |
|
@jamesaoverton Fine with me. :) But for the record, I then plan to enable the option by default in ODK workflows (users would still have the possibility to disable it in their ODK configuration). |
|
I really like this general pattern (robot is rigorous about backward
compatibility, ODK may override)
…On Mon, Mar 10, 2025 at 2:23 PM Damien Goutte-Gattat < ***@***.***> wrote:
@jamesaoverton <https://github.com/jamesaoverton> Fine with me. :) But
for the record, I then plan to enable the option by default in ODK
workflows (users would still have the possibility to disable it in their
ODK configuration).
—
Reply to this email directly, view it on GitHub
<#1248 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAMMOPF7NJGJR6YU55OFLT2TX7FNAVCNFSM6AAAAABYP5IHXCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMJRHA3TIMBUGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: gouttegd]*gouttegd* left a comment (ontodev/robot#1248)
<#1248 (comment)>
@jamesaoverton <https://github.com/jamesaoverton> Fine with me. :) But
for the record, I then plan to enable the option by default in ODK
workflows (users would still have the possibility to disable it in their
ODK configuration).
—
Reply to this email directly, view it on GitHub
<#1248 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAMMOPF7NJGJR6YU55OFLT2TX7FNAVCNFSM6AAAAABYP5IHXCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMJRHA3TIMBUGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This request has been implemented: the old behaviour remains the default.
Resolves [#1014, #1208]
docs/have been added/updatedmvn verifysays all tests passmvn sitesays all JavaDocs correctCHANGELOG.mdhas been updatedThis PR amends the Reduce code to include
SubObjectPropertyOfandSubPropertyChainOfaxioms in the graph used to evaluate redundant axioms.By including
SubObjectPropertyOfaxioms, if we have something likethen the first
SubClassOfaxiom can be considered redundant and removed (#1208).By including
SubPropertyChainOfaxioms, if we havethen the second
SubClassOfaxiom (SubClassOf(C, P some E)) can be considered redundant and removed (#1014).