Breaking: changed signature of ProblemInspector.GetAllPossibleSubstit…

…utions for consistency.

Also some TODO updates

src/SCClassicalPlanning.Alternatives/Planning/GraphPlan/GraphPlanPlanningTask_FromAIaMA.cs

@@ -199,7 +199,7 @@ public IEnumerator<SearchEdge> GetEnumerator()
             var relevantActionNodes = goal.Elements
                 .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.
+                .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:
             // We go recursive to ensure that we ultimately find all combinations - but this is of course potentially expensive.

src/SCClassicalPlanning.Documentation/wwwroot/md/getting-started.md

@@ -17,8 +17,8 @@ In this section, we use the ['blocks world'](https://en.wikipedia.org/wiki/Block
 ### Defining Problems as Code
 
 ```
-using SCClassicalPlanning; // for Goal, Effect, Action, Problem, IState, HashSetState
-using SCFirstOrderLogic; // for Constant, Term, Predicate, VariableDeclaration, EqualitySymbol
+using SCClassicalPlanning; // for Problem, HashSetState, Goal, Action, Effect
+using SCFirstOrderLogic; // for Constant, Term, Predicate, VariableDeclaration, EqualityIdentifier
 using static SCFirstOrderLogic.SentenceCreation.OperableSentenceFactory; // for OperablePredicate
 using Action = SCClassicalPlanning.Action; // an unfortunate clash with System.Action. I'd rather not rename it..
 
@@ -27,15 +27,15 @@ using Action = SCClassicalPlanning.Action; // an unfortunate clash with System.A
 // helper methods for your predicates, as we do below, is highly recommended to avoid repetition.
 // NB #1: note that we're using OperablePredicate here. Its not required, but makes everything nice and succinct because
 // it means we can use & and ! to combine them. See the SCFirstOrderLogic docs for details.
-// NB #2: Also note the use of EqualityIdnetifier.Instance here to identify the equality predicate. While at present there's
+// NB #2: Also note the use of EqualityIdentifier.Instance here to identify the equality predicate. While at present there's
 // no special handling of equality in any of the algorithms (in here or in SCFirstOrderLogic), its worth using this anyway
 // - even if only for future-proofing.
 OperablePredicate On(Term above, Term below) => new Predicate(nameof(On), above, below);
 OperablePredicate Block(Term block) => new Predicate(nameof(Block), block);
 OperablePredicate Clear(Term surface) => new Predicate(nameof(Clear), surface);
 OperablePredicate Equal(Term x, Term y) => new Predicate(EqualityIdentifier.Instance, x, y);
 
-// First, let's declare the initial state of our problem. IState is an interface that represents a set of
+// First, let's define the initial state of our problem. IState is an interface that represents a set of
 // predicates that can be modified by applying Actions. For problems with large states, a state implementation
 // that is backed by a separate store might be required, but here we have a small enough problem that just
 // keeping everything in memory is fine. The library includes an implementation of IState called HashSetState
@@ -58,8 +58,8 @@ HashSetState initialState = new(
     & Clear(blockC));
 
 // Next, let's define the end goal of our problem. Goals are essentially sets of literals.
-// Note well that goals are sets of *literals*, not predicates. That is, we can require that
-// certain predicates do *not* hold. In this case though, both of our goal's elements are positive.
+// Note that, unlike states, that is sets of *literals*, not predicates. This is so that we can require
+// that certain predicates do *not* hold. In this case though, both of our goal's elements are positive.
 Goal endGoal = new(
     On(blockA, blockB)
     & On(blockB, blockC));

src/SCClassicalPlanning/Action.cs

@@ -68,7 +68,7 @@ public class Action
     /// </para>
     /// <para>
     /// NB: Does NOT validate preconditions - to be of use with particular planning heuristics.
-    /// TODO: Maybe rethink this. Any heuristic that wants to do this could just look at the effect and apply it..
+    /// TODO-ROBUSTNESS: Maybe rethink this. Any heuristic that wants to do this could just look at the effect and apply it..
     /// </para>
     /// </summary>
     /// <param name="state">The state to apply the action to.</param>

src/SCClassicalPlanning/HashSetState.cs

@@ -71,7 +71,7 @@ public HashSetState(Sentence sentence) : this(ConstructionVisitor.Visit(sentence
     /// </summary>
     public ImmutableHashSet<Predicate> Elements { get; }
 
-    // TODO: at some point look at (test me!) efficiency here. ImmutableHashSet builder stuff might be of use?
+    // TODO-PERFORMANCE: at some point look at (test me!) efficiency here. ImmutableHashSet builder stuff might be of use?
     /// <inheritdoc />
     IState IState.Apply(Effect effect) => new HashSetState(Elements.Except(effect.DeleteList).Union(effect.AddList));
 

src/SCClassicalPlanning/IState.cs

@@ -52,7 +52,7 @@ public interface IState
     /// </summary>
     /// <param name="goal">The goal to check.</param>
     /// <returns>A value indicating whether this state satisfies a given goal.</returns>
-    // TODO*: if we're keeping Elements, not necessarily needed, because can do this with queries? Could add a default implementation, at least?
+    // TODO*: not necessarily needed, because can do this with queries? Could add a default implementation, at least?
     bool Satisfies(Goal goal);
 
     /// <summary>

src/SCClassicalPlanning/Planning/StateAndGoalSpace/CostStrategies/UnsatisfiedGoalCount.cs

@@ -36,8 +36,7 @@ public class UnsatisfiedGoalCount : ICostStrategy
     /// <inheritdoc/>
     public float EstimateCost(IState state, Goal goal)
     {
-        // TODO: Do states need to be i(async)queryable?
-        //return 0f;
+        // TODO: leverage the fact that state.elements are iqueryable now
         return goal.RequiredPredicates.Except(state.Elements).Count()
             + goal.ForbiddenPredicates.Intersect(state.Elements).Count();
     }

src/SCClassicalPlanning/Planning/Utilities/ProblemInspector.cs

@@ -81,7 +81,7 @@ IEnumerable<VariableSubstitution> MatchWithState(IEnumerable<Literal> goalElemen
                     // one would hope that it is a very rare scenario to have a variable that doesn't occur in ANY positive goal elements (most
                     // will at least occur in e.g. a 'type' predicate). But this is obviously VERY expensive when it occurs - though I guess
                     // clever indexing could help (support for indexing is TODO).
-                    foreach (var firstGoalElementUnifier in GetAllPossibleSubstitutions(firstGoalElement.Predicate, unifier, state.GetAllConstants()))
+                    foreach (var firstGoalElementUnifier in GetAllPossibleSubstitutions(firstGoalElement.Predicate, state.GetAllConstants(), unifier))
                     {
                         var possiblePredicate = firstGoalElementUnifier.ApplyTo(firstGoalElement.Predicate);
 
@@ -169,7 +169,7 @@ IEnumerable<VariableSubstitution> ExpandNonMatchesWithGoalNegation(IEnumerable<L
                 // do not occur, we need to check for the existence of the negation of the literal formed by substituting EVERY combination of
                 // objects in the problem for the as yet unbound variables. This is obviously VERY expensive for large problems with lots of objects -
                 // though I guess clever indexing could help (support for indexing is TODO).
-                foreach (var firstEffectElementUnifier in GetAllPossibleSubstitutions(firstEffectElement.Predicate, substitution, constants))
+                foreach (var firstEffectElementUnifier in GetAllPossibleSubstitutions(firstEffectElement.Predicate, constants, substitution))
                 {
                     var possibleLiteral = firstEffectElementUnifier.ApplyTo(firstEffectElement);
 
@@ -407,7 +407,7 @@ IEnumerable<VariableSubstitution> MatchWithSchema(Action action, Action schema,
     /// <returns>All possible subsitutions that populate each of the arguments of the given predicate with a constant.</returns>
     public static IEnumerable<VariableSubstitution> GetAllPossibleSubstitutions(Predicate predicate, IEnumerable<Constant> constants)
     {
-        return GetAllPossibleSubstitutions(predicate, new VariableSubstitution(), constants);
+        return GetAllPossibleSubstitutions(predicate, constants, new VariableSubstitution());
     }
 
     /// <summary>
@@ -420,10 +420,10 @@ public static IEnumerable<VariableSubstitution> GetAllPossibleSubstitutions(Pred
     /// </para>
     /// </summary>
     /// <param name="predicate">The predicate to create all possible substitutions for.</param>
-    /// <param name="constraint">The substitution to use as a constraint.</param>
     /// <param name="constants">The available constants.</param>
+    /// <param name="constraint">The substitution to use as a constraint.</param>
     /// <returns>All possible subsitutions that populate each of the arguments of the given predicate with a constant.</returns>
-    public static IEnumerable<VariableSubstitution> GetAllPossibleSubstitutions(Predicate predicate, VariableSubstitution constraint, IEnumerable<Constant> constants)
+    public static IEnumerable<VariableSubstitution> GetAllPossibleSubstitutions(Predicate predicate, IEnumerable<Constant> constants, VariableSubstitution constraint)
     {
         IEnumerable<VariableSubstitution> allPossibleSubstitutions = new List<VariableSubstitution>() { constraint };
         var unboundVariables = predicate.Arguments.OfType<VariableReference>().Except(constraint.Bindings.Keys);
@@ -433,7 +433,6 @@ public static IEnumerable<VariableSubstitution> GetAllPossibleSubstitutions(Pred
             allPossibleSubstitutions = allPossibleSubstitutions.SelectMany(
                 // NB the AsEnumerable here to reduce requirements on what the IQueryable
                 // implementation needs to support. Still unconvinced about using IQueryable, TBH.
-                // TODO* depending on what i do with Problem constants - might need to look in problem.Domain, too.
                 u => constants.Select(o => u.CopyAndAdd(KeyValuePair.Create(unboundVariable, (Term)o)))
             );
         }

src/SCClassicalPlanning/Problem.cs

@@ -18,7 +18,7 @@ namespace SCClassicalPlanning;
 /// Encapsulates a planning problem.
 /// </para>
 /// <para>
-/// Problems consist of an initial <see cref="IState"/>, an end <see cref="Goal"/>, and a set of allowed <see cref="Action"/>s.
+/// Problems consist of an initial <see cref="IState"/>, an end <see cref="Goal"/>, and a set of schemas for allowed <see cref="Action"/>s.
 /// </para>
 /// </summary>
 public class Problem

src/SCClassicalPlanning/ProblemManipulation/RecursiveStateVisitor{TState}.cs

@@ -15,7 +15,6 @@
 
 namespace SCClassicalPlanning.ProblemManipulation;
 
-// TODO*: probably straight up remove this class. In general, iterating all elements of a state isn't going to be viable. 
 /// <summary>
 /// Base class for recursive visitors of <see cref="IState"/> instances that reference external visitation state
 /// (as opposed to maintaining state as fields of the visitor).