Skip to content

Fixed DFS traversal - #18

Open
sebdumancic wants to merge 4 commits into
ztangent:masterfrom
Herb-AI:master
Open

Fixed DFS traversal#18
sebdumancic wants to merge 4 commits into
ztangent:masterfrom
Herb-AI:master

Conversation

@sebdumancic

Copy link
Copy Markdown

The DFS traversal did not return the answers in the same order as a Prolog implementation like SWIPL.
This is now fixed.

@ztangent

Copy link
Copy Markdown
Owner

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 ztangent left a comment

Copy link
Copy Markdown
Owner

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!

Comment thread src/main.jl
# 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}()

Copy link
Copy Markdown
Owner

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.

Copy link
Copy Markdown
Owner

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.

Comment thread src/main.jl
# 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

Copy link
Copy Markdown
Owner

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?

Comment thread src/main.jl
while (search==:dfs) && length(local_queue) > 0
pushfirst!(queue, pop!(local_queue))
end

Copy link
Copy Markdown
Owner

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?

Copy link
Copy Markdown
Owner

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_clauses

with

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.

@sebdumancic sebdumancic Mar 31, 2022

Copy link
Copy Markdown
Author

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.

@ztangent

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants