Fix constraint checking in Forward and BreadthFirstSearch planners

[aemartinez]

No description provided.

[aemartinez]

Upon closer inspection, I realized that constraints are also not being checked in the initial state of the search. I believe the best place for this check is the solve function. Let me know if this makes sense and I'll make the changes.

[ztangent]

Thanks for catching both of these issues! I think adding an is_violated check to the solve function is the right approach. For ForwardPlanner, it'd be great if you could add the check right before line 381 in the implementation of refine! as well --- this is another situation where search gets restarted from the initial state.

SymbolicPlanners.jl/src/planners/forward.jl

Lines 370 to 392 in 27364c4

	function refine!(
	sol::PathSearchSolution{S, T}, planner::ForwardPlanner,
	domain::Domain, state::State, spec::Specification
	) where {S, T <: PriorityQueue}
	@unpack heuristic, refine_method, reset_node_count = planner
	# Resimplify goal specification and ensure heuristic is precomputed
	spec = simplify_goal(spec, domain, state)
	ensure_precomputed!(heuristic, domain, state, spec)
	# Decide between restarting, rerooting, or continuing the search
	if refine_method == :restart
	(sol.status == :failure && is_reached(state, sol)) && return sol
	reinit_sol!(sol, planner, heuristic, domain, state, spec)
	elseif refine_method == :reroot
	reroot!(sol, planner, heuristic, domain, state, spec)
	sol.status = :in_progress
	elseif refine_method == :continue
	(sol.status == :success || sol.status == :failure) && return sol
	sol.status = :in_progress
	end
	planner.reset_node_count && (sol.expanded = 0)
	# Run search and return solution
	return search!(sol, planner, heuristic, domain, spec)
	end

[aemartinez]

I've been testing this on the BackwardPlanner and found some potential issues.

MethodError if using StateConstrainedGoal

I'm unsure what's the adequate way of specifying a constrained goal in the context of the backward planner. The following code just uses StateConstrainedGoal and yields a MethodError.

using PDDL, SymbolicPlanners
domain = @pddl(
   "(define (domain d)
    (:predicates 
        (robot-at ?loc)
    )
    (:action move
        :parameters  (?from ?to)
        :precondition (and  (robot-at ?from))
        :effect (and 
            (robot-at ?to) (not (robot-at ?from))
        )
    )
    )"
)
problem = @pddl(
   "(define (problem p) (:domain d)
    (:objects a b)
    (:init (robot-at a))
    (:goal (robot-at b))
    (:constraints (and 
        (not (robot-at a))
    ))
    )"
)
planner = BackwardPlanner()
init_state = initstate(domain, problem)
goal_spec = StateConstrainedGoal(problem)
sol = planner(domain, init_state, goal_spec)

ERROR: MethodError: add_constraints!(::BackwardSearchGoal{StateConstrainedGoal{MinStepsGoal}, PDDL.GenericDiff}, ::GenericDomain, ::GenericState) is ambiguous.

Using instead BackwardSearchGoal does not crash but still gives an incorrect result.

(domain and problem defined as above.)

using PDDL, SymbolicPlanners
domain = ...
problem = ...
planner = BackwardPlanner()
init_state = initstate(domain, problem)
goal_spec = StateConstrainedGoal(problem)
spec = BackwardSearchGoal(goal_spec, init_state)
sol = planner(domain, init_state, spec)
display(sol)

Output:

status: :success
  plan: 1-element Vector{Term}
    (move a b)

Note that the constraint is only violated in the first state of the trajectory (the last state of the search). Even though the constraint is added to the state on line 228, it seems the solution is not discarded.

SymbolicPlanners.jl/src/planners/backward.jl

Lines 224 to 228 in 6126004

	for act in filter_relevant(heuristic, domain, state, spec)
	# Regress (reverse-execute) the action
	next_state = regress(domain, state, act; check=false)
	# Add constraints to regression state
	add_constraints!(spec, domain, next_state)

[ztangent]

Thanks! I think this issue overlaps with the one raised in #6. It seems like the method error might be due to how I currently have add_constraints! implemented, which currently leads to method ambiguity.

SymbolicPlanners.jl/src/specifications/backward.jl

Lines 53 to 59 in 27364c4

	function add_constraints!(
	spec::BackwardSearchGoal{G,C}, domain::Domain, state::State
	) where {G,C <: PDDL.Diff}
	update!(state, domain, spec.constraint_diff)
	end
	add_constraints!(spec::BackwardSearchGoal, domain::Domain, state::State) =
	nothing

I think the issue would be fixed by changing the type signature for the first method to have a more specific type, thereby avoiding ambiguity about which of the two methods apply.

function add_constraints!(
    spec::BackwardSearchGoal{G,C}, domain::Domain, state::State
) where {G <: StateConstrainedGoal, C <: PDDL.Diff}
    update!(state, domain, spec.constraint_diff)
end

I think fixing the method ambiguity issue is separate from the other issue you discovered. I think the second issue is due to the fact that add_constraints! currently doesn't handle negation properly. The current implementation of BackwardPlanner relies on the assumption that a set of states (or equivalently, an abstract state) can be compactly represented by the set of predicates that hold true across all states in that set. This is why we can just use the PDDL.GenericState datatype to represent abstract states, even though it's normally meant to represent concrete states. However, this means we can't easily represent abstract states that are defined by some predicate being false for all states in that set.

I think it will take some refactoring of the underlying PDDL codebase for me to properly support abstract state representations in all their generality. In the meantime, it'd be great if you could try testing a case where the constraints only contain positive literals, and no negative literals. Hopefully the planner will do the correct thing in that case. We can also update the docstring to clarify what BackwardPlanner currently supports or does not support.

[aemartinez]

After changing the suggested type signature, the ambiguity is resolved, and the correct method is invoked. However, this results in an UndefVarError as update! is not recognized.

Is update! implemented anywhere? If not, the best course of action for now may be to simply document that constraints are not supported in BackwardPlanner yet.

[ztangent]

update! is supposed to be implemented in PDDL.jl! But I wouldn't worry about fixing this right now, and documenting that it is not currently supported sounds good to me.

[aemartinez]

But I wouldn't worry about fixing this right now, and documenting that it is not currently supported sounds good to me.

Sounds good 👍. I've added a sentence to the planner's documentation.