-
Notifications
You must be signed in to change notification settings - Fork 4
Add get_contrastive_explanation() support in SyncReasoner + integration tests in (Feature/abduction sync reasoner) #167
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
base: develop
Are you sure you want to change the base?
Conversation
owlapy/owl_reasoner.py
Outdated
| j_manager = self.ontology.owlapi_manager # OWLOntologyManager (java) | ||
| j_ontology = self.ontology.owlapi_ontology # OWLOntology (java) | ||
| mapper = self.ontology.mapper # bridge mapper | ||
| j_reasoner = getattr(self, "owlapi_reasoner", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to store them in local variables when you can use self.x directly?
Also why call getattr(self, "owlapi_reasoner", None). The owlapi reasoner is stored under self._owlapi_reasoner and is never None.
owlapy/owl_reasoner.py
Outdated
| # --------------------------------------------------------- | ||
| # 3. (Optional) warm up the shared reasoner we already have | ||
| # --------------------------------------------------------- | ||
| if j_reasoner is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the owlapi_reasoner is never None
owlapy/owl_reasoner.py
Outdated
| if str(reasoner_name).lower() == "hermit": | ||
| j_choice = ReasonerChoice.HERMIT | ||
| else: | ||
| j_choice = ReasonerChoice.ELK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the same reasoner that the user has defined (stored self.reasoner_name). For example the user may have specified Pellet but your code will default to ELK for ExplenationConfig.
alkidbaci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
When I try to run the example script
ontology_contrastive_explanation.pyI get a java exception:Traceback (most recent call last): File "/home/alkid/PycharmProjects/owlapy/examples/ontology_contrastive_explanation.py", line 64, in <module> main() File "/home/alkid/PycharmProjects/owlapy/examples/ontology_contrastive_explanation.py", line 26, in main ontology = SyncOntology(ontology_path) File "/home/alkid/PycharmProjects/owlapy/owlapy/owl_ontology.py", line 1021, in __init__ self.owlapi_ontology = self.owlapi_manager.loadOntologyFromOntologyDocument(File(file_path)) java.lang.java.lang.NullPointerException: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "parserFactory" is nullThis is happening when trying to initialize a
SyncOntologyobject. The exception does not happen in the develop branch. -
The tests are not passing successfully. Please see into that.
-
I have left you some comments in the code changes
-
Your tests only ensure type and format correctness but not what the result actually contains. For example, instead of just checking
self.assertIsInstance(result["common"], set), you can also checkself.assertCountEqual(result["common"],expected_result)whereexpected_resultis a set containing the expected result. -
Please move the methods you have implemented (
get_contrastive_explanationandcreate_justifications) after the__init__of SyncReasoner. Ideally you put them at the end, which indicates a natural order of implementation time. -
Please rename the jar dependency
contrastive-explanations-0.3-SNAPSHOT-jar-with-dependencies.jar(remove the"-SNAPSHOT-jar-with-dependencies"part)
429e5b7 to
ceb51cf
Compare
|
@copilot Check if all the changes requested are taken care of, otherwise open a new PR and complete them. |
|
@alkidbaci I've opened a new pull request, #174, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
It appears that the jar dependency that you have added is overwriting some of the existing java packages. |
|
We do need de.tu-dresden.inf.lat.evee:evee-elk-proof-extractor-owlapi4:0.3 (OWLAPI4-based) for proof extraction. Running mvn dependency:tree -Dverbose shows that this dependency pulls in ELK OWLAPI4 artifacts (and transitively OWLAPI 4.5.9), while the owlapy project itself depends on OWLAPI 5.x. Since both OWLAPI 4 and OWLAPI 5 use overlapping org.semanticweb.owlapi.* packages, this can lead to class shadowing when building the fat jar. de.tu-dresden.inf.lat.evee:evee-elk-proof-extractor-owlapi4:0.3 Then contrast it with our owlapy project OWLAPI5 usage: anonymized:contrastive-explanations:jar:0.3-SNAPSHOT |
|
Well I believe this is a problem with no apparent solution then. The JVM can only be started once and the dependencies are set at the moment of initiation. The conflicting distributions of OWLAPI should not coexist in the same JVM session. |
This pull request introduces support for contrastive (fact–foil) explanations in SyncReasoner, using the updated Java explanation generator. It also includes integration tests to validate end-to-end behavior with a real ontology and HermiT reasoner.
What was added :