Skip to content

Conversation

@sebdumancic
Copy link

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
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.

Copy link
Owner

@ztangent ztangent left a 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}()
Copy link
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
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.

# 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
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?

while (search==:dfs) && length(local_queue) > 0
pushfirst!(queue, pop!(local_queue))
end

Copy link
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
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.

Copy link
Author

@sebdumancic sebdumancic Mar 31, 2022

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
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