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

OWL2SPARQL Converter ForAll Alternative #39

Merged
merged 4 commits into from
May 10, 2024

Conversation

nkaralis
Copy link
Contributor

@nkaralis nkaralis commented May 8, 2024

Introduces an alternative for the ForAll mapping based on De Morgan's laws.

NOTE: The interface (arguments) of the converter have changed. Might break some stuff in Ontolearn

@nkaralis nkaralis requested a review from Demirrr May 8, 2024 09:24
@Demirrr Demirrr requested a review from alkidbaci May 8, 2024 10:05
@Demirrr
Copy link
Member

Demirrr commented May 8, 2024

@alkidbaci Could you review it today if you have time ?

@alkidbaci
Copy link
Collaborator

Sure thing

Copy link
Collaborator

@alkidbaci alkidbaci left a comment

Choose a reason for hiding this comment

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

@nkaralis I noticed that you have commented outline 128-129 and added line 274-277 instead. The class argument named_individuals was applicable for every conversion but according to your changes, now its applicable only for concepts of type OWLObjectComplementOf.

One more thing to notice here is that we cannot specify the argument for_all_de_morgan in the method owl_expression_to_sparql but since that argument is only relevant to concepts of type OWLObjectAllValuesFrom maybe it's better to keep it this way. If someone wants to specify a different value for for_all_de_morgan they can create a new instance of Owl2SparqlConverter and call as_query from there.

Other than these, it looks fine to me.

@nkaralis
Copy link
Contributor Author

nkaralis commented May 8, 2024

Thanks for reviewing the PR.

Regarding the first point, the idea here is that OWLObjectComplementOf iterates over all entities of the graph, whereas other expressions are restricted based on a type.
We could also remove the comments and have both.

Regarding the second point, we could add an extra argument de_morgan_for_all: bool = True. What do you think?

@alkidbaci
Copy link
Collaborator

Regarding the first point, the idea here is that OWLObjectComplementOf iterates over all entities of the graph, whereas other expressions are restricted based on a type.

Well, that's a fair point. We can leave it like that then.

Regarding the second point, we could add an extra argument de_morgan_for_all: bool = True. What do you think?

Now that I think again about this, people will get used to using the owl_expression_to_sparql method and that's also the way we recommend in the documentation on how to convert owl to sparql... so I suggest adding that argument. Hope you don't mind doing that.

Thanks for the PR!

@alkidbaci alkidbaci merged commit 20f4a61 into develop May 10, 2024
3 checks passed
@alkidbaci alkidbaci deleted the owl2sparql-forall-alternative branch August 8, 2024 15:14
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

Successfully merging this pull request may close these issues.

3 participants