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

Added reasoning to the creation of oeo-full.omn #1465

Merged
merged 11 commits into from
Dec 1, 2023

Conversation

areleu
Copy link
Contributor

@areleu areleu commented Feb 1, 2023

Summary of the discussion

Related to #1124

Type of change (CHANGELOG.md)

Added

  • Reasoner is called after building the full ontology

Workflow checklist

Automation

Closes #

PR-Assignee

Reviewer

  • 🐙 Follow the Reviewer Guide
  • 🐙 Provided feedback and show sufficient appreciation for the work done

@areleu
Copy link
Contributor Author

areleu commented Feb 8, 2023

#1475 has to be merged first

@areleu
Copy link
Contributor Author

areleu commented Feb 14, 2023

Can not be inferred:

  • DisjointClass: Probably some incongruent disjoint class assertion.
  • PropertyAssertion: Probably some missused property in the instances.

Edit: Tried to infer Disjoint classes in Protegé and it went kaputt

@areleu areleu requested a review from MGlauer February 22, 2023 14:39
@areleu
Copy link
Contributor Author

areleu commented Feb 24, 2023

After thinking a lot about this I am now starting to wonder if it is really necessary to provide the Disjoint class inferences as part of the ontology with full closure. Is there any example application where one would need this? I accept hypothetical examples as well.

@l-emele
Copy link
Contributor

l-emele commented Feb 24, 2023

We definitely do not need every disjoint. For example, in PR #882 we added the axioms:

'typical day' 'has part' some ('time span' and ('has unit' some day) and ('has number' value 1))
'typical year' 'has part' some ('time span' and ('has unit' some year) and ('has number' value 1))

Before the mentioned PR we had only:
'typical day' DisjointWith: 'typical year'

But this example shows nicely that we sometimes need disjoints at least temporarily. In the example, the disjoint should have been deleted when the proper axioms were included.

@areleu
Copy link
Contributor Author

areleu commented Feb 24, 2023

We definitely do not need every disjoint. For example, in PR #882 we added the axioms:

'typical day' 'has part' some ('time span' and ('has unit' some day) and ('has number' value 1))
'typical year' 'has part' some ('time span' and ('has unit' some year) and ('has number' value 1))

Before the mentioned PR we had only: 'typical day' DisjointWith: 'typical year'

But this example shows nicely that we sometimes need disjoints at least temporarily. In the example, the disjoint should have been deleted when the proper axioms were included.

I think Disjoints are non avoidable. But I mean if we need to provide the inference of all the disjoint classses of every class (which is performed in the closure of the ontology when adding the DisjointClasses option), this is the operation that I think takes insanely long and I actually don't see any direct use case.

@areleu
Copy link
Contributor Author

areleu commented Mar 9, 2023

Just for everyones information:

phillord/hermit-reasoner#13 (comment)

The algorithm is by itself slow for the amount of classes we have in the ontology. So we might have to consider providing the disjoint classes by some other means i don't really know how long would the reasoning would take but I suspect it may be too much for github. So probalby we can use some internal server for that, either from Magdeburg or somewhere else.

@areleu areleu changed the title Added reasoning to the creation of oeo-full.omn Draft: Added reasoning to the creation of oeo-full.omn Mar 23, 2023
@stap-m stap-m marked this pull request as draft March 23, 2023 09:14
@stale stale bot added the stale already discussed issues that haven't got worked on for a while label Apr 7, 2023
@areleu
Copy link
Contributor Author

areleu commented May 22, 2023

We could merge this under two considerations:

  • The inferred ontology is created under the name ontology-closure.owl
  • We do not consider DisjointClass amnd PropertyAssertion reasoning

@stale stale bot removed the stale already discussed issues that haven't got worked on for a while label May 22, 2023
@stale stale bot added stale already discussed issues that haven't got worked on for a while and removed stale already discussed issues that haven't got worked on for a while labels Jun 10, 2023
@areleu
Copy link
Contributor Author

areleu commented Jun 15, 2023

Open issues moved to #1583

@areleu areleu changed the title Draft: Added reasoning to the creation of oeo-full.omn Added reasoning to the creation of oeo-full.omn Jun 15, 2023
@areleu areleu requested a review from stap-m June 15, 2023 09:38
@MGlauer
Copy link
Contributor

MGlauer commented Jul 28, 2023

I do not really understand why additional subsumptions are inferred only to be dropped by robot reduce in the next step. Am I mistaken in assuming that the aim of oeo-closure that it contains the transitive closure of the subsumption relation?

@areleu
Copy link
Contributor Author

areleu commented Aug 1, 2023

I do not really understand why additional subsumptions are inferred only to be dropped by robot reduce in the next step. Am I mistaken in assuming that the aim of oeo-closure that it contains the transitive closure of the subsumption relation?

I pretty much built it based on https://github.com/OpenEnergyPlatform/ontology/pull/1125/files and since reduce was getting rid of duplicate axioms just left it there. Didn't know it was also getting rid of desired inferences.

@stale stale bot added the stale already discussed issues that haven't got worked on for a while label Sep 16, 2023
@stale stale bot removed the stale already discussed issues that haven't got worked on for a while label Oct 23, 2023
@areleu
Copy link
Contributor Author

areleu commented Dec 1, 2023

I removed the reduce command, seems to be woking. I looked at a couple of entities and it seems to be inferring things correctly. Some axioms seem redundant but I guess that is the point of the operation. From my side we could merge this but I really think it would be nice to have someone take a second look.

Copy link
Contributor

@MGlauer MGlauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem good, the oeo-full ontology seems consistent and opens in protege. Good from my side!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants