Fixes #519: Knowledge Gap bug with semi-duplicate triples#594
Fixes #519: Knowledge Gap bug with semi-duplicate triples#594
Conversation
bnouwt
left a comment
bnouwt
left a comment
There was a problem hiding this comment.
Hey @Sophietje, thanks for the changes 👍 I finished the review. Please do not consider the comments to be very important and feel free to choose to ignore them. I think it is useful to just mention most things that come up during a review although not all of them are equally relevant 😄
| for (Entry<RuleNode, Set<Match>> entry : someAntecedentNeighbors.entrySet()) { | ||
| for (Match m : entry.getValue()) { | ||
| if (m.getMatchingPatterns().keySet().contains(tp)) { | ||
| if (m.getMatchingPatterns().containsValue(tp)) { |
There was a problem hiding this comment.
@Sophietje Do you remember why this change was necessary? Because the removed line and the added line do not check the same thing. I vaguely remember looking at this together, but do not remember the reason.
There was a problem hiding this comment.
I don't quite remember. If I change it back to keySet(), the testKnowledgeGapNoMatchingVars() exits with the knowledge gap: [?iq hasImage ?image, ?iq type ImperialFaberge, ?iq madeIn ?country, ?iq madeBy ?company] whereas we originally defined the expected knowledge gap to be: [?id madeIn Russia, ?id commissionedBy Alexander III, ?id madeBy House of Fabergé]
smart-connector/src/main/java/eu/knowledge/engine/smartconnector/impl/ReasonerProcessor.java
Outdated
Show resolved
Hide resolved
smart-connector/src/main/java/eu/knowledge/engine/smartconnector/impl/ReasonerProcessor.java
Show resolved
Hide resolved
| private static TripleMatchType getTripleMatchType(TriplePattern existingTriple, TriplePattern newTriple) { | ||
| Map<TripleNode, TripleNode> matches = existingTriple.findMatches(newTriple); | ||
| if (matches == null) { | ||
| return TripleMatchType.ADD_TRIPLE; |
There was a problem hiding this comment.
I always try to have only a single return statement per method, because this increases readability in many cases. Not a necessary change, just want to mention it.
There was a problem hiding this comment.
In this specific case I think I disagree. I'd prefer a clear return thus indicating end of this method for an (easy) case if it's at the beginning of a method, as opposed to needing to read the rest of the method to see whether anything else happens. I do agree that the return later on is less neat and I'll fix that.
smart-connector/src/main/java/eu/knowledge/engine/smartconnector/impl/ReasonerProcessor.java
Show resolved
Hide resolved
smart-connector/src/main/java/eu/knowledge/engine/smartconnector/impl/ReasonerProcessor.java
Outdated
Show resolved
Hide resolved
smart-connector/src/main/java/eu/knowledge/engine/smartconnector/impl/TripleMatchType.java
Outdated
Show resolved
Hide resolved
smart-connector/src/main/java/eu/knowledge/engine/smartconnector/impl/TripleMatchType.java
Outdated
Show resolved
Hide resolved
| private AskKnowledgeInteraction askKI; | ||
| private Set<Rule> ruleSet; | ||
|
|
||
| @BeforeEach |
There was a problem hiding this comment.
I notice you are executing this setup() method @BeforeEach test method. Is that necessary? Because it makes the test quite a bit slower, I think. Or is it because some method create additional Knowledge Interactions that you do want lying around for subsequent tests?
There was a problem hiding this comment.
Mostly that second part, this way it's clear which KIs are supposed to be part of which test case, not adding any unnnecessary clutter.
...onnector/src/test/java/eu/knowledge/engine/smartconnector/api/TestKnowledgeGapDetection.java
Show resolved
Hide resolved
2 participants
There's a bug in
getKnowledgeGaps()where a gap is returned with two nearly-identical triples, e.g.:?id hasCountry ?countryand?id hasCountry Ireland. We only want to keep the?id hasCountry Irelandin this case and ignore the triple with the additional variable.