Skip to content

Ebr#147

Merged
alkidbaci merged 20 commits into
developfrom
EBR
Jul 7, 2025
Merged

Ebr#147
alkidbaci merged 20 commits into
developfrom
EBR

Conversation

@LckyLke

@LckyLke LckyLke commented Jun 17, 2025

Copy link
Copy Markdown
Collaborator

added embedding based reasoner with inference support on gpu and batch size parameter (how many distinct predictions, e.g. (h,r,?) to run at once)

@Demirrr

Demirrr commented Jun 17, 2025

Copy link
Copy Markdown
Member

Thank you! @alkidbaci could you proceed with the reviewing process?

@LckyLke

LckyLke commented Jun 17, 2025

Copy link
Copy Markdown
Collaborator Author

I will add some tests before we merge :)

@Demirrr

Demirrr commented Jun 17, 2025

Copy link
Copy Markdown
Member

Thank you:)

@LckyLke

LckyLke commented Jun 17, 2025

Copy link
Copy Markdown
Collaborator Author

I think we need to make a new release for dice-embeddings as EBR uses the new KGE implementation that is only on development branch rn

@alkidbaci

Copy link
Copy Markdown
Collaborator

@Demirrr sorry to ask for your time while you are in vacation but if you can, give me a pypi api token for dicee and I'll take care of it

@Demirrr

Demirrr commented Jun 17, 2025

Copy link
Copy Markdown
Member

No worries. I will add you @alkidbaci

Comment thread owlapy/neural_ontology.py Outdated

@alkidbaci alkidbaci left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Although this is already mentioned here #143, looking at the code like this makes me feel really uncomfortable. You already implement and use methods such as individuals_in_signature , classes_in_signature, etc. that one usually finds in an implementation of AbstractOntology. I'm thinking if there is really no other way of doing this in a more sensible manner. Like for example to implement the core of EBR in the class NeuralOntology, enough to implement the methods of AbstractOntology and everything else you can implement on EBR itself. You can always set the necessary attributes in NeuralOntology to later use them in EBR.

Please let me know if what I'm saying make sense to you @LckyLke.

Also for the moment I think is better that we keep all the reasoners in the owl_reasoner.py module and ontologies in owl_ontology.py because we either implement each reasoner in a separate module or we keep them all together in the same one. The latter makes it easier to import/find reasoners

@LckyLke

LckyLke commented Jun 20, 2025

Copy link
Copy Markdown
Collaborator Author

Although this is already mentioned here #143, looking at the code like this makes me feel really uncomfortable. You already implement and use methods such as individuals_in_signature , classes_in_signature, etc. that one usually finds in an implementation of AbstractOntology. I'm thinking if there is really no other way of doing this in a more sensible manner. Like for example to implement the core of EBR in the class NeuralOntology, enough to implement the methods of AbstractOntology and everything else you can implement on EBR itself. You can always set the necessary attributes in NeuralOntology to later use them in EBR.

Please let me know if what I'm saying make sense to you @LckyLke.

Also for the moment I think is better that we keep all the reasoners in the owl_reasoner.py module and ontologies in owl_ontology.py because we either implement each reasoner in a separate module or we keep them all together in the same one. The latter makes it easier to import/find reasoners

Regarding your first point:
As for the EBR approach, all methods require the usage of the predict method as we dont want to have the graph in memory at any point. So as all methods have a similar structure, I think in makes sense to have them all in this class to avoid redundancies.

@alkidbaci

Copy link
Copy Markdown
Collaborator

all methods require the usage of the predict method

I dont see the issue here. You can have the predict method in NeuralOntology and call it from there, right?

So as all methods have a similar structure, I think in makes sense to have them all in this class to avoid redundancies.

What is the redundancy here exactly?

In the end I think its a trade-off between how much we adhere to the standards already set in the framework and the amount of redundancies that you are referring to. What I'm trying to point out here and get this clear so we can find this appropriate "trade-off" is that taken from a user's perspective who knows that an implementer of AbstractOntology has a method classes_in_signature for example calls this method from a NeuralOntology object, he will be greeted with a NotImplementedError and most likely unaware that this method is already available, but on the EBR. You can say that this ontology is there just to use EBR, why would one want to use it separately? So in short, you are forced to have this ontology implementation because of how reasoners are set up in owlapy and since they are set up like that for a reason I think its better to adhere to it as long as it is possible otherwise that means that either EBR is not a "reasoner" (by what we define a reasoner in owlapy) or our way of setting up things is wrong which can also be the case but in my understanding, considering also how things are structured in OWLAPI, the method asking for signature information should belong to the ontology.

@LckyLke

LckyLke commented Jun 20, 2025

Copy link
Copy Markdown
Collaborator Author

I dont see the issue here. You can have the predict method in NeuralOntology and call it from there, right?

Yes we can do that.
And I unstaid what you mean by following like the "usual structure". For the EBR specifically the split is than a bit arbitrary of which method belongs where, but for the overall consistency it definitely makes sense.

→ I can update the implementation

@alkidbaci

Copy link
Copy Markdown
Collaborator

Great, if there is something I can do to help, please let me know

@alkidbaci alkidbaci merged commit a0ab7ea into develop Jul 7, 2025
2 checks passed
@alkidbaci alkidbaci deleted the EBR branch July 7, 2025 13:12
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