-
Notifications
You must be signed in to change notification settings - Fork 9
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
TripleStoreKnowledgeBase vs TripleStore #451
Comments
KnowledgeBase implements the following computation in the initialization
see here We need to think about bringing them together @alkidbaci @LckyLke |
Yes I do agree that a refactoring is needed here and we should have a single triple store KB class. The suggested combined class should ideally still inherit from |
Should consider finding a solution for #446 also |
Is there any reason why the ind_set is not just simply initialized like this?:
|
Not that I am aware of 😀 |
To understand the code base of these classes better, I wrote a small script to visualize the common and unique methods of the classes so we can identify possible merging points etc. |
Also, some functions with the same name in these classes operate differently: If you look at get_all_sub_concepts from TripleStoreKnowledgeBase for instance: def get_all_sub_concepts(self, concept: OWLClassExpression) -> Iterable[OWLClassExpression]:
assert isinstance(concept, OWLClass)
yield from self.reasoner.sub_classes(concept, direct=False) and from TripleStore: def get_all_sub_concepts(self, concept: OWLClass, direct=True):
yield from self.reasoner.subconcepts(concept, direct) one has direct to false and you cant change it, and the other has it to true by default but it is changeable. |
So I was thinking a bit about this and this is my proposed solution: First PartWe have this class Second PartWith that structure set to place, we can then merge Third PartSince our CEL algorithms currently accept as an argument only Other remarksI am currently handling some comments and TODOs in the I would like to know your opinions and of course if you have any other suggestions. P.S: I may work on this for the next few Mondays (since I have taken no vacation on Mondays) if I get the green line to continue with it by @Demirrr and if there is no other prioritized work to do. |
First PartAgreed! Second PartMostly agreed. Currently, class TripleStoreKnowledgeBase(KnowledgeBase): . Therefore, I do not think that Third PartAgreed! Plan
For the sake of efficient programming and writing tests, we can do the following as well In the construct of |
Shall we combine these classes into one ?
The text was updated successfully, but these errors were encountered: