-
Notifications
You must be signed in to change notification settings - Fork 131
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
N3 Reasoner Class #296
N3 Reasoner Class #296
Conversation
I just wanted to jot a few top-of-mind notes around a couple of modifications that should be made down the line to improve the features & performance of reasoning. I'm not suggesting we do this now.
|
const [val0, val1, val2] = rule.premise[i].value, index = content[rule.premise[i].content]; | ||
const v0 = !(value = val0.value); | ||
for (value in v0 ? index : { [value]: index[value] }) { | ||
if (index1 = index[value]) { |
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.
Note for self when I get around to finish cleaning this up. Due to the way that N3.js handles deletions; this is only here for the case where value is defined and !(value in index). So we can reduce a bunch of unnecessary calcs by moving this check outside of the loop.
One option is
for (value in v0 ? index : value in index ? { [value]: index[value] } : {})
...
@jeswr I would love to see this code in N3. I have immediate plans to use this in some webapps I have in mind. Do you have in the reasoner support for native N3 lists? In my own attempts I've struggled with that. Dörthe Arndt created a test case for me a while ago. How would this be expressed in the N3 Reasoner?
|
As would I, however, I am currently undecided on whether the API should be as it is at present, or, whether That said it shouldn't be much work for you to change between the two usages so you could always just use the branch in its current state in your app by running
Not in the current state of this PR, as that would require the capability to be able to mint new blank nodes in order to represent the list. Adding this capability shouldn't be too difficult, but I would prefer to look into how other reasoners do it and think about the best way of encoding that in rules so as to not limit ourselves from future performance improvements. |
@jeswr thanks this works. I see that the
|
+1 on Edit: I see the current approach already uses composition, which is good IMO :-) |
It's certainly feasible, though it may incur a performance hit. I'll see if I can give this a go over the weekend (no promises though). There are, again, some API decisions to be made around this - but at the very least I would consider adding support for multiple stores as input at the same time as this would require the same additions to the logic, as that required to apply the sem-naive algorithm across the |
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
This introduces an N3 Reasoner class as discussed in #295
For the record here is the output of the performance test, noting that my machine has ~20% variance in results as noted in past PRs