graphplan fixes. getting there - a couple of test cases pass now.

src/SCClassicalPlanning.ExampleProblems/FromAIaMA/SpareTire.cs

@@ -41,11 +41,6 @@ static SpareTire()
         public static Constant Axle { get; } = new(nameof(Axle));
         public static Constant Trunk { get; } = new(nameof(Trunk));
 
-        /// <summary>
-        /// Gets the implicit state of the world, that will never change as the result of actions.
-        /// </summary>
-        public static OperableSentence ImplicitState => IsTire(Spare) & IsTire(Flat);
-
         public static OperablePredicate IsTire(Term tire) => new Predicate(nameof(IsTire), tire);
 
         public static OperablePredicate IsAt(Term item, Term location) => new Predicate(nameof(IsAt), item, location);

src/SCClassicalPlanning.Tests/Planning/GraphPlan/GraphPlanTests.cs

@@ -1,64 +1,29 @@
-////using FluentAssertions;
-////using FlUnit;
-////using SCClassicalPlanning.ExampleDomains.FromAIaMA;
-////using SCFirstOrderLogic;
-////using static SCClassicalPlanning.ExampleDomains.FromAIaMA.BlocksWorld;
+using FluentAssertions;
+using FlUnit;
+using SCClassicalPlanning.ExampleDomains.FromAIaMA;
+using SCFirstOrderLogic;
+using static SCClassicalPlanning.ExampleDomains.FromAIaMA.BlocksWorld;
 
-////namespace SCClassicalPlanning.Planning.GraphPlan
-////{
-////    public static class GraphPlanTests
-////    {
-////        public static Test CreatedPlanValidity => TestThat
-////            .GivenTestContext()
-////            .AndEachOf(() => new Problem[]
-////            {
-////                AirCargo.ExampleProblem,
-////                BlocksWorld.ExampleProblem,
-////                SpareTire.ExampleProblem,
-////                MakeBigBlocksWorldProblem(),
-////            })
-////            .When((_, tc) =>
-////            {
-////                var planner = new GraphPlan();
-////                return planner.CreatePlan(tc);
-////            })
-////            .ThenReturns()
-////            .And((_, tc, pl) => tc.Goal.IsSatisfiedBy(pl.ApplyTo(tc.InitialState)).Should().BeTrue())
-////            .And((cxt, tc, pl) => cxt.WriteOutputLine(new PlanFormatter(tc.Domain).Format(pl)));
-
-////        private static Problem MakeBigBlocksWorldProblem()
-////        {
-////            Constant blockA = new(nameof(blockA));
-////            Constant blockB = new(nameof(blockB));
-////            Constant blockC = new(nameof(blockC));
-////            Constant blockD = new(nameof(blockD));
-////            Constant blockE = new(nameof(blockE));
-
-////            return BlocksWorld.MakeProblem(
-////                initialState: new(
-////                    Block(blockA)
-////                    & Equal(blockA, blockA)
-////                    & Block(blockB)
-////                    & Equal(blockB, blockB)
-////                    & Block(blockC)
-////                    & Equal(blockC, blockC)
-////                    & Block(blockD)
-////                    & Equal(blockD, blockD)
-////                    & Block(blockE)
-////                    & Equal(blockE, blockE)
-////                    & On(blockA, Table)
-////                    & On(blockB, Table)
-////                    & On(blockC, blockA)
-////                    & On(blockD, blockB)
-////                    & On(blockE, Table)
-////                    & Clear(blockD)
-////                    & Clear(blockE)
-////                    & Clear(blockC)),
-////                goal: new(
-////                    On(blockA, blockB)
-////                    & On(blockB, blockC)
-////                    & On(blockC, blockD)
-////                    & On(blockD, blockE)));
-////        }
-////    }
-////}
+namespace SCClassicalPlanning.Planning.GraphPlan
+{
+    public static class GraphPlanTests
+    {
+        public static Test CreatedPlanValidity => TestThat
+            .GivenTestContext()
+            .AndEachOf(() => new Problem[]
+            {
+                SpareTire.ExampleProblem,
+                //AirCargo.ExampleProblem,
+                BlocksWorld.ExampleProblem,
+                //BlocksWorld.LargeExampleProblem,
+            })
+            .When((_, tc) =>
+            {
+                var planner = new GraphPlan();
+                return planner.CreatePlan(tc);
+            })
+            .ThenReturns()
+            .And((_, tc, pl) => tc.Goal.IsSatisfiedBy(pl.ApplyTo(tc.InitialState)).Should().BeTrue())
+            .And((cxt, tc, pl) => cxt.WriteOutputLine(new PlanFormatter(tc.Domain).Format(pl)));
+    }
+}

src/SCClassicalPlanning.Tests/Planning/GraphPlan/PlanningGraphTests.cs

@@ -117,12 +117,5 @@ static IEnumerable<Predicate> GetPositiveTireIsAtPropositions(PlanningGraph g, i
                 IsAt(Flat, Axle),
                 IsAt(Flat, Ground),
             }));
-
-
-        // NB: yeah, pretty terrible coverage (testing large outputs is generally a PITA)
-        // note that it gets indirectly tested via the PlanningGraph heuristics tests.
-        // and will be futher tests via graphplan, when that's done.
-        // Will also extend at some point - should at least completely cover the cake example -
-        // which i think has only 2 levels..
     }
 }

src/SCClassicalPlanning/Planning/GraphPlan/GraphPlan.cs

@@ -140,7 +140,8 @@ private bool TryExtractSolution(
 
                 if (search.IsSucceeded)
                 {
-                    plan = new Plan(search.PathToTarget().Reverse().SelectMany(e => e.Actions).ToList());
+                    // TODO: eliminate magic string
+                    plan = new Plan(search.PathToTarget().Reverse().SelectMany(e => e.Actions).Where(a => !a.Identifier.Equals("NOOP")).ToList());
                     return true;
                 }
                 else
@@ -186,12 +187,12 @@ public SearchNode(PlanningGraph.Level graphLevel, Goal goal)
 
         private readonly struct SearchNodeEdges : IReadOnlyCollection<SearchEdge>
         {
-            private readonly PlanningGraph.Level level;
+            private readonly PlanningGraph.Level graphLevel;
             private readonly Goal goal;
 
-            public SearchNodeEdges(PlanningGraph.Level level, Goal goal)
+            public SearchNodeEdges(PlanningGraph.Level graphLevel, Goal goal)
             {
-                this.level = level;
+                this.graphLevel = graphLevel;
                 this.goal = goal;
             }
 
@@ -219,18 +220,18 @@ public IEnumerator<SearchEdge> GetEnumerator()
                  * 2. To achieve that literal, prefer actions with easier preconditions.That is, choose an action such that the sum (or maximum) of the level costs of its preconditions is smallest."
                  */
 
-                var level = this.level; // copy level because we can't access struct instance field in lambda
+                var graphLevel = this.graphLevel; // copy level variable because we can't access struct instance field in lambda
 
                 // Order the goal elements by descending level cost:
                 var goalElements = goal.Elements
-                    .OrderByDescending(e => level.Graph.GetLevelCost(e));
+                    .OrderByDescending(e => graphLevel.Graph.GetLevelCost(e));
 
                 // Find all of the actions that satisfy at least one element of the goal and order by sum of precondition level costs:
                 // NB: While we use the term "relevant" here, note that we're not discounting those that clash with the goal - that will 
                 // be dealt with by mutex checks.
                 var relevantActionNodes = goal.Elements
-                    .SelectMany(e => level.NodesByProposition[e].Causes)
-                    .OrderBy(n => n.Preconditions.Sum(p => level.Graph.GetLevelCost(p.Proposition)))
+                    .SelectMany(e => graphLevel.NodesByProposition[e].Causes)
+                    .OrderBy(n => n.Preconditions.Sum(p => graphLevel.Graph.GetLevelCost(p.Proposition)))
                     .Distinct(); // todo: can probably do this before order by? ref equality, but we take action to avoid dups.
 
                 // Now (recursively) attempt to cover all elements of the goal, with no mutexes:
@@ -242,7 +243,7 @@ public IEnumerator<SearchEdge> GetEnumerator()
             IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
 
             private IEnumerable<SearchEdge> Recurse(
-                IEnumerable<Literal> goalElements,
+                IEnumerable<Literal> unsatisfiedGoalElements,
                 ImmutableHashSet<PlanningGraph.ActionNode> unselectedActionNodes,
                 ImmutableHashSet<PlanningGraph.ActionNode> selectedActionNodes)
             {
@@ -260,23 +261,23 @@ bool IsNonMutexWithSelectedActions(PlanningGraph.ActionNode actionNode)
                 }
 
                 // TODO-PERFORMANCE: a lot of GC pressure here, what with all the immutable hash sets.
-                // could eliminate the duplication by creating a tree instead (or a BitArray/BitVector32 to indicate selection).
+                // could eliminate the duplication by creating a tree instead, or just some bit vector struct to indicate selection.
                 // meh, lets get it working first, then at least we have a baseline.
-                if (!goalElements.Any())
+                if (!unsatisfiedGoalElements.Any())
                 {
-                    yield return new SearchEdge(level, selectedActionNodes.Select(n => n.Action));
+                    yield return new SearchEdge(graphLevel, goal, selectedActionNodes.Select(n => n.Action));
                 }
                 else
                 {
                     // Try to cover the first goal element (any others covered by the same action are a bonus)
-                    var firstGoalElement = goalElements.First();
+                    var firstGoalElement = unsatisfiedGoalElements.First();
 
                     foreach (var actionNode in unselectedActionNodes)
                     {
                         if (actionNode.Action.Effect.Elements.Contains(firstGoalElement) && IsNonMutexWithSelectedActions(actionNode))
                         {
                             foreach (var edge in Recurse(
-                                goal.Elements.Except(actionNode.Action.Effect.Elements),
+                                unsatisfiedGoalElements.Except(actionNode.Action.Effect.Elements),
                                 unselectedActionNodes.Remove(actionNode),
                                 selectedActionNodes.Add(actionNode)))
                             {
@@ -291,19 +292,19 @@ bool IsNonMutexWithSelectedActions(PlanningGraph.ActionNode actionNode)
         private readonly struct SearchEdge : IEdge<SearchNode, SearchEdge>
         {
             private readonly PlanningGraph.Level graphLevel;
+            private readonly Goal goal;
 
-            public SearchEdge(PlanningGraph.Level graphLevel, IEnumerable<Action> actions)
+            public SearchEdge(PlanningGraph.Level graphLevel, Goal goal, IEnumerable<Action> actions)
             {
                 this.graphLevel = graphLevel;
+                this.goal = goal;
                 this.Actions = actions;
             }
 
             public IEnumerable<Action> Actions { get; }
 
             /// <inheritdoc />
-            // Could implement as new(planningGraph, fromGoal); - but we'd need fromGoal, which is a waste.
-            // Private struct and unused property, so just ignore it.
-            public SearchNode From => throw new NotImplementedException(); 
+            public SearchNode From => new(graphLevel, goal);
 
             /// <inheritdoc />
             public SearchNode To => new(graphLevel.PreviousLevel!, new Goal(Actions.SelectMany(a => a.Precondition.Elements)));

src/SCClassicalPlanning/Planning/GraphPlan/PlanningGraph.cs

@@ -17,6 +17,7 @@
 using SCFirstOrderLogic.SentenceManipulation;
 using SCFirstOrderLogic.SentenceManipulation.Unification;
 using System.Collections.ObjectModel;
+using System.Diagnostics;
 
 namespace SCClassicalPlanning.Planning.GraphPlan
 {
@@ -429,6 +430,7 @@ public bool ContainsNonMutex(IEnumerable<Literal> propositions)
         /// it query it via graph theoretical algorithms - so it would be needless complexity. Easy enough to change
         /// should we ever want to do that (probably just by layering some structs over the top of these existing classes).
         /// </summary>
+        [DebuggerDisplay("{Proposition}")]
         public class PropositionNode
         {
             internal PropositionNode(Literal proposition) => Proposition = proposition;
@@ -452,6 +454,7 @@ public class PropositionNode
         /// it query it via graph theoretical algorithms - so it would be needless complexity. Easy enough to change
         /// should we ever want to do that (probably just by layering some structs over the top of these existing classes).
         /// </summary>
+        [DebuggerDisplay("{Action.Identifier}: {Action.Effect}")]
         public class ActionNode
         {
             internal ActionNode(Action action) => Action = action;