-
Notifications
You must be signed in to change notification settings - Fork 11
Fixed DFS traversal #18
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: master
Are you sure you want to change the base?
Conversation
|
Getting to this a bit slow because I was on vacation last week! Thanks for fixing the DFS issue -- I have a couple of questions before merging, just to make sure I understand what's going on, will ask in them in a PR review. |
ztangent
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.
Thanks for these changes! I suspect there's a way to accomplish the same behavior (of matching the SWIPL search order) without using a local queue, and without using a MutableLinkedList. I can try making those changes by adding a commit to this PR, but first I think it'd be a good idea to add a test case (e.g. a simple Prolog program that uses cut), so that we can test that Julog's behavior matches the SWIPL behavior. It'd be great if you could do that!
| # Construct top level goal and put it on the queue | ||
| queue = [GoalTree(Const(false), nothing, Vector{Term}(goals), 1, env, Subst())] | ||
| subst = [] | ||
| queue = MutableLinkedList{GoalTree}() |
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.
Is there a reason why you switched to using MutableLinkedLists? I believe Julia arrays have (amortized) constant time for pushes and pops at both the start and end of the array (see this thread), so I would only want to switch to a MutableLinkedList if there are clear performance gains on the existing benchmarks.
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.
I think if we're going to change data structures (which I'd do as a separate PR), it seems better to use a Deque, which appears to have faster performance for pushfirst! and popfirst!: https://juliacollections.github.io/DataStructures.jl/latest/deque/
However, I think this doesn't really matter for DFS, since we can rewrite everything to use only push! and pop! from the end of a Vector.
| # Iterate across clause set with matching heads | ||
| matched_clauses = retrieve_clauses(clauses, term, funcs) | ||
| matched = false | ||
| local_queue = MutableLinkedList{GoalTree}() # maintain a local queue for the depth first search; the new goals should be added to the global queue in this order to the front |
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.
I worry about creating a local queue leading to performance slow downs because we're now allocating a queue at every iteration of the search -- is there a way to avoid using a local queue?
| while (search==:dfs) && length(local_queue) > 0 | ||
| pushfirst!(queue, pop!(local_queue)) | ||
| end | ||
|
|
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.
Instead of pushing each child to the local queue, then transferring the items to the global queue, why we just pushfirst! directly to the global queue?
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.
Oh, after giving it more thought, I think I understand why doing it this way leads to different behavior.
Rather than using a local queue though, I think what we can do is replace the line:
for c in matched_clauseswith
for c in Iterators.reverse(matched_clauses)This will iterate over the matched clauses in reverse order, allowing us to pushfirst! directly to the global queue.
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.
Right, this is a much better solution! Better not to change data structures if not necessary.
I was searching for the most efficient data structure for front/back adding, but I like this solution much more.
|
After some further thought, I realized that the real reason why Julog's depth-first search order differs from SWI-Prolog is because it's not actually implementing backtracking search. As far as I can tell, in standard implementations of Prolog, you're supposed to immediate start working on the next goal once you find a matching clause, and only move on to the next matching clause (trying to unify it with the goal, etc.) once you're done with the current clause. What Julog is currently doing in DFS is that it's adding iterating through all matching clauses, checking if they unify, and adding them to the queue, before popping goals off from the end of the queue --- i.e., it adds all children to the queue, before descending to the next level. To fix this, I think I'll have to reimplement the search procedure so that backtracking search is actually possible. This will hopefully lead to some memory improvements as well, but it'll take a bit of work and refactoring. |
The DFS traversal did not return the answers in the same order as a Prolog implementation like SWIPL.
This is now fixed.