FVI comments, docs and minor private member refactoring

src/SCFirstOrderLogic/ClauseIndexing/AsyncFeatureVectorIndexDictionaryNode{TFeature,TValue}.cs

@@ -2,9 +2,7 @@
 // You may use this file in accordance with the terms of the MIT license.
 using SCFirstOrderLogic.SentenceManipulation.VariableManipulation;
 using System;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Dynamic;
 using System.Threading.Tasks;
 
 namespace SCFirstOrderLogic.ClauseIndexing;
@@ -30,7 +28,9 @@ public class AsyncFeatureVectorIndexDictionaryNode<TFeature, TValue> : IAsyncFea
     private readonly Dictionary<CNFClause, TValue> valuesByKey = new(new VariableIdIgnorantEqualityComparer());
 
     /// <summary>
-    /// Initialises a new instance of the <see cref="AsyncFeatureVectorIndexDictionaryNode{TFeature, TValue}"/> class.
+    /// Initialises a new instance of the <see cref="AsyncFeatureVectorIndexDictionaryNode{TFeature, TValue}"/> class that
+    /// uses the default comparer of the feature type to determine the ordering of nodes. Note that this comparer will
+    /// throw if the runtime type of a feature object does not implement <see cref="IComparable{T}"/>.
     /// </summary>
     public AsyncFeatureVectorIndexDictionaryNode()
         : this(Comparer<TFeature>.Default)
@@ -41,10 +41,9 @@ public AsyncFeatureVectorIndexDictionaryNode()
     /// Initialises a new instance of the <see cref="AsyncFeatureVectorIndexDictionaryNode{TFeature, TValue}"/> class.
     /// </summary>
     /// <param name="featureComparer">
-    /// The comparer to use to determine the ordering of features when adding to the index and performing
-    /// queries. NB: For correct behaviour, the index must be able to unambiguously order the components
-    /// of a feature vector. As such, this comparer must only return zero for equal features (and of course 
-    /// duplicates shouldn't occur in any given vector).
+    /// The comparer to use to determine the ordering of nodes. NB: For correct behaviour, the index must be able to
+    /// unambiguously order the components of a feature vector. As such, this comparer must only return zero for equal 
+    /// features (and of course duplicates shouldn't occur in any given vector).
     /// </param>
     public AsyncFeatureVectorIndexDictionaryNode(IComparer<TFeature> featureComparer)
     {

src/SCFirstOrderLogic/ClauseIndexing/AsyncFeatureVectorIndex{TFeature,TValue}.cs

@@ -79,7 +79,7 @@ public async Task AddAsync(CNFClause key, TValue value)
         }
 
         var currentNode = root;
-        foreach (var vectorComponent in MakeOrderedFeatureVector(key))
+        foreach (var vectorComponent in MakeAndSortFeatureVector(key))
         {
             currentNode = await currentNode.GetOrAddChildAsync(vectorComponent);
         }
@@ -96,7 +96,7 @@ public async Task<bool> RemoveAsync(CNFClause key)
     {
         ArgumentNullException.ThrowIfNull(key);
 
-        var featureVector = MakeOrderedFeatureVector(key);
+        var featureVector = MakeAndSortFeatureVector(key);
 
         return await ExpandNodeAsync(root, 0);
 
@@ -136,7 +136,7 @@ async ValueTask<bool> ExpandNodeAsync(IAsyncFeatureVectorIndexNode<TFeature, TVa
         ArgumentNullException.ThrowIfNull(key);
 
         var currentNode = root;
-        foreach (var element in MakeOrderedFeatureVector(key))
+        foreach (var element in MakeAndSortFeatureVector(key))
         {
             var childNode = await currentNode.TryGetChildAsync(element);
             if (childNode != null)
@@ -161,7 +161,7 @@ public IAsyncEnumerable<TValue> GetSubsuming(CNFClause clause)
     {
         ArgumentNullException.ThrowIfNull(clause);
 
-        var featureVector = MakeOrderedFeatureVector(clause);
+        var featureVector = MakeAndSortFeatureVector(clause);
 
         return ExpandNode(root, 0);
 
@@ -192,7 +192,7 @@ async IAsyncEnumerable<TValue> ExpandNode(IAsyncFeatureVectorIndexNode<TFeature,
             else
             {
                 // NB: note that we need to filter the values to those keyed by clauses that actually
-                // subsume the query clause. The node values are the candidate set.
+                // subsume the query clause. The node values are just the *candidate* set.
                 await foreach (var value in node.KeyValuePairs.Where(kvp => kvp.Key.Subsumes(clause)).Select(kvp => kvp.Value))
                 {
                     yield return value;
@@ -210,7 +210,7 @@ public IAsyncEnumerable<TValue> GetSubsumed(CNFClause clause)
     {
         ArgumentNullException.ThrowIfNull(clause);
 
-        var featureVector = MakeOrderedFeatureVector(clause);
+        var featureVector = MakeAndSortFeatureVector(clause);
 
         return ExpandNode(root, 0);
 
@@ -250,7 +250,7 @@ async IAsyncEnumerable<TValue> ExpandNode(IAsyncFeatureVectorIndexNode<TFeature,
         async IAsyncEnumerable<TValue> GetAllDescendentValues(IAsyncFeatureVectorIndexNode<TFeature, TValue> node)
         {
             // NB: note that we need to filter the values to those keyed by clauses that are
-            // actually subsumed by the query clause. The node values are the candidate set.
+            // actually subsumed by the query clause. The node values are just the *candidate* set.
             await foreach (var value in node.KeyValuePairs.Where(kvp => clause.Subsumes(kvp.Key)).Select(kvp => kvp.Value))
             {
                 yield return value;
@@ -266,8 +266,9 @@ async IAsyncEnumerable<TValue> GetAllDescendentValues(IAsyncFeatureVectorIndexNo
         }
     }
 
-    private IList<FeatureVectorComponent<TFeature>> MakeOrderedFeatureVector(CNFClause clause)
+    private IList<FeatureVectorComponent<TFeature>> MakeAndSortFeatureVector(CNFClause clause)
     {
+        // todo-performance: if we need a list anyway, probably faster to make the list, then sort it in place? test me
         return featureVectorSelector(clause).OrderBy(kvp => kvp.Feature, root.FeatureComparer).ToList();
     }
 }

src/SCFirstOrderLogic/ClauseIndexing/FeatureVectorIndexDictionaryNode{TFeature,TValue}.cs

@@ -21,7 +21,8 @@ public class FeatureVectorIndexDictionaryNode<TFeature, TValue> : IFeatureVector
 
     /// <summary>
     /// Initialises a new instance of the <see cref="FeatureVectorIndexDictionaryNode{TFeature, TValue}"/> class that
-    /// uses the default comparer of the feature type to determine the ordering of nodes.
+    /// uses the default comparer of the feature type to determine the ordering of nodes. Note that this comparer will
+    /// throw if the runtime type of a feature object does not implement <see cref="IComparable{T}"/>.
     /// </summary>
     public FeatureVectorIndexDictionaryNode()
         : this(Comparer<TFeature>.Default)

src/SCFirstOrderLogic/ClauseIndexing/FeatureVectorIndex{TFeature,TValue}.cs

@@ -22,8 +22,6 @@ namespace SCFirstOrderLogic.ClauseIndexing;
 public class FeatureVectorIndex<TFeature, TValue>
     where TFeature : notnull
 {
-    private static readonly IEnumerable<KeyValuePair<CNFClause, TValue>> EmptyElements = Enumerable.Empty<KeyValuePair<CNFClause, TValue>>();
-
     private readonly Func<CNFClause, IEnumerable<FeatureVectorComponent<TFeature>>> featureVectorSelector;
     private readonly IFeatureVectorIndexNode<TFeature, TValue> root;
 
@@ -36,7 +34,7 @@ public class FeatureVectorIndex<TFeature, TValue>
     public FeatureVectorIndex(
         Func<CNFClause, IEnumerable<FeatureVectorComponent<TFeature>>> featureVectorSelector,
         IFeatureVectorIndexNode<TFeature, TValue> root)
-        : this(featureVectorSelector, root, EmptyElements)
+        : this(featureVectorSelector, root, Enumerable.Empty<KeyValuePair<CNFClause, TValue>>())
     {
     }
 
@@ -80,7 +78,7 @@ public void Add(CNFClause key, TValue value)
         }
 
         var currentNode = root;
-        foreach (var component in MakeOrderedFeatureVector(key))
+        foreach (var component in MakeAndSortFeatureVector(key))
         {
             currentNode = currentNode.GetOrAddChild(component);
         }
@@ -97,7 +95,7 @@ public bool Remove(CNFClause key)
     {
         ArgumentNullException.ThrowIfNull(key);
 
-        var featureVector = MakeOrderedFeatureVector(key);
+        var featureVector = MakeAndSortFeatureVector(key);
 
         return ExpandNode(root, 0);
 
@@ -137,7 +135,7 @@ public bool TryGet(CNFClause key, [MaybeNullWhen(false)] out TValue value)
         ArgumentNullException.ThrowIfNull(key);
 
         var currentNode = root;
-        foreach (var vectorComponent in MakeOrderedFeatureVector(key))
+        foreach (var vectorComponent in MakeAndSortFeatureVector(key))
         {
             if (currentNode.TryGetChild(vectorComponent, out var childNode))
             {
@@ -162,12 +160,12 @@ public IEnumerable<TValue> GetSubsuming(CNFClause clause)
     {
         ArgumentNullException.ThrowIfNull(clause);
 
-        var featureVector = MakeOrderedFeatureVector(clause);
+        var featureVector = MakeAndSortFeatureVector(clause);
 
         return ExpandNode(root, 0);
 
         // NB: subsumed clauses will have equal or higher vector elements. So subsuming clauses will have equal or lower vector elements.
-        // We allow zero-valued elements to be omitted from the vectors (so that we don't have to know what identifiers are possible ahead of time).
+        // We allow zero-valued elements to be omitted from the vectors (so that we don't have to know what features are possible ahead of time).
         // This makes the logic here a little similar to what you'd find in a set trie when querying for subsets.
         IEnumerable<TValue> ExpandNode(IFeatureVectorIndexNode<TFeature, TValue> node, int componentIndex)
         {
@@ -196,7 +194,7 @@ IEnumerable<TValue> ExpandNode(IFeatureVectorIndexNode<TFeature, TValue> node, i
             else
             {
                 // NB: note that we need to filter the values to those keyed by the clauses that
-                // actually subsume the query clause. The values of the node are the candidate set.
+                // actually subsume the query clause. The values of the node are just the *candidate* set.
                 foreach (var value in node.KeyValuePairs.Where(kvp => kvp.Key.Subsumes(clause)).Select(kvp => kvp.Value))
                 {
                     yield return value;
@@ -214,7 +212,7 @@ public IEnumerable<TValue> GetSubsumed(CNFClause clause)
     {
         ArgumentNullException.ThrowIfNull(clause);
 
-        var featureVector = MakeOrderedFeatureVector(clause);
+        var featureVector = MakeAndSortFeatureVector(clause);
 
         return ExpandNode(root, 0);
 
@@ -257,7 +255,7 @@ IEnumerable<TValue> ExpandNode(IFeatureVectorIndexNode<TFeature, TValue> node, i
         IEnumerable<TValue> GetAllDescendentValues(IFeatureVectorIndexNode<TFeature, TValue> node)
         {
             // NB: note that we need to filter the values to those keyed by clauses that are
-            // actually subsumed by the query clause. The values of the node are the candidate set.
+            // actually subsumed by the query clause. The values of the node are just the *candidate* set.
             foreach (var value in node.KeyValuePairs.Where(kvp => clause.Subsumes(kvp.Key)).Select(kvp => kvp.Value))
             {
                 yield return value;
@@ -273,8 +271,9 @@ IEnumerable<TValue> GetAllDescendentValues(IFeatureVectorIndexNode<TFeature, TVa
         }
     }
 
-    private IList<FeatureVectorComponent<TFeature>> MakeOrderedFeatureVector(CNFClause clause)
+    private IList<FeatureVectorComponent<TFeature>> MakeAndSortFeatureVector(CNFClause clause)
     {
+        // todo-performance: if we need a list anyway, probably faster to make the list, then sort it in place? test me
         return featureVectorSelector(clause).OrderBy(kvp => kvp.Feature, root.FeatureComparer).ToList();
     }
 }