changeset 33003:c9d869d9c989

Simplify code complexity of perms.cc (bug #65244) * perms.cc (GetPerms): Remove third input "do_sort". Merge code from GetPermsNoSort(). Use std::iota rather than hand-rolled for loop. * perms.cc (GetPermsNoSort): Delete function. * perms.cc (Fperms): Rename functions calls to GetPermsNoSort() to GetPerms(). Rewrite input validation error message to refer to input "V".
author koerhen <koerhen@web.de>
date Sat, 03 Feb 2024 11:04:44 +0800
parents 4fb466fb717e
children 9c9f4df5e4c3
files libinterp/corefcn/perms.cc
diffstat 1 files changed, 21 insertions(+), 82 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/perms.cc	Sat Feb 10 18:41:10 2024 -0800
+++ b/libinterp/corefcn/perms.cc	Sat Feb 03 11:04:44 2024 +0800
@@ -28,6 +28,7 @@
 #endif
 
 #include <algorithm>
+#include <numeric>
 
 #include "defun.h"
 #include "error.h"
@@ -48,103 +49,42 @@
 //
 // Use C++ template to cater for the different octave array classes.
 //
+
 template <typename T>
 static inline Array<T>
-GetPerms (const Array<T>& ar_in, bool uniq_v, bool do_sort = false)
+GetPerms (const Array<T>& ar_in, bool uniq_v = false)
 {
   octave_idx_type m = ar_in.numel ();
   double nr = Factorial (m);
 
   // Setup index vector filled from 0..m-1
   OCTAVE_LOCAL_BUFFER (int, myvidx, m);
-  for (int i = 0; i < m; i++)
-    myvidx[i] = i;
-
-  // Interim array to sort ar_in for octave sort order and to implement
-  // "unique".
-  Array<T> ar (ar_in);
-
-  if (uniq_v)
-    {
-      ar = ar.sort (ar.dims () (1) > ar.dims () (0) ? 1 : 0, ASCENDING);
-      const T *Ar = ar.data ();
-      int ctr = 0;
-      int N_el = 1;
-
-      // Number of same elements where we need to remove permutations
-      // Number of unique permutations is n! / (n_el1! * n_el2! * ...)
-      for (octave_idx_type i = 0; i < m - 1; i++)
-        {
-          myvidx[i] = ctr;
-          if (Ar[i + 1] != Ar[i])
-            {
-              nr /= Factorial (N_el);
-              ctr = i + 1;  // index of next different element
-              N_el = 1;
-            }
-          else
-            N_el++;
-        }
-      myvidx[m - 1] = ctr;
-      nr /= Factorial (N_el);
-    }
-  else if (do_sort)
-    {
-      ar = ar.sort (ar.dims () (1) > ar.dims () (0) ? 1 : 0, ASCENDING);
-    }
-
-  // Sort vector indices for inverse lexicographic order later.
-  std::sort (myvidx, myvidx + m, std::greater<int> ());
-
-  const T *Ar = ar.data ();
-
-  // Set up result array
-  octave_idx_type n = static_cast<octave_idx_type> (nr);
-  Array<T> res (dim_vector (n, m));
-  T *Res = res.rwdata ();
-
-  // Do the actual job
-  octave_idx_type i = 0;
-  std::sort (myvidx, myvidx + m, std::greater<int> ());
-  do
-    {
-      for (octave_idx_type j = 0; j < m; j++)
-        Res[i + j * n] = Ar[myvidx[j]];
-      i++;
-    }
-  while (std::next_permutation (myvidx, myvidx + m, std::greater<int> ()));
-
-  return res;
-}
-
-// Template for non-numerical types (e.g. Cell) without sorting.
-// The C++ compiler complains as the provided type octave_value does not
-// support the test of equality via '==' in the above template.
-
-template <typename T>
-static inline Array<T>
-GetPermsNoSort (const Array<T>& ar_in, bool uniq_v = false)
-{
-  octave_idx_type m = ar_in.numel ();
-  double nr = Factorial (m);
-
-  // Setup index vector filled from 0..m-1
-  OCTAVE_LOCAL_BUFFER (int, myvidx, m);
-  for (int i = 0; i < m; i++)
-    myvidx[i] = i;
+  std::iota (&myvidx[0], &myvidx[m], 0);
 
   const T *Ar = ar_in.data ();
 
   if (uniq_v)
     {
-      // Mutual Comparison using is_equal to detect duplicated values
+      // Mutual Comparison is used to detect duplicated values.
+      // Using sort would be possible for numerical values and be of 
+      // O(n*log (n)) complexity instead of O(n * (n / 2)).  But sort
+      // is not supported for the octave_value container (structs/cells). 
+      // As the perms element size n must very small, any potential gains
+      // would be minimal as nearly all CPU time is spent to create the 
+      // actual permutations.
+
       int N_el = 1;
       // Number of unique permutations is n! / (n_el1! * n_el2! * ...)
       for (octave_idx_type i = 0; i < m - 1; i++)
         {
           for (octave_idx_type j = i + 1; j < m; j++)
             {
-              if (myvidx[j] > myvidx[i] && Ar[i].is_equal (Ar[j]))
+              bool isequal;
+              if constexpr (std::is_same<T, octave_value>::value)
+                isequal = Ar[i].is_equal (Ar[j]);
+              else
+                isequal = (Ar[i] == Ar[j]);
+              if (myvidx[j] > myvidx[i] && isequal)
                 {
                   myvidx[j] = myvidx[i];  // not yet processed...
                   N_el++;
@@ -259,8 +199,7 @@
          || args (0).iscell () || args (0).is_scalar_type ()
          || args (0).isstruct ()))
     {
-      error ("perms: INPUT must be a matrix, a range, a cell array, "
-             "a struct or a scalar.");
+      error ("perms: input V must be a matrix, range, cell array, struct, or scalar.");
     }
 
   std::string clname = args (0).class_name ();
@@ -291,7 +230,7 @@
   else if (clname == "uint64")
     retval = GetPerms<octave_uint64> (args (0).uint64_array_value (), uniq_v);
   else if (clname == "cell")
-    retval = GetPermsNoSort<octave_value> (args (0).cell_value (), uniq_v);
+    retval = GetPerms<octave_value> (args (0).cell_value (), uniq_v);
   else if (clname == "struct")
     {
       const octave_map map_in (args (0).map_value ());
@@ -312,7 +251,7 @@
             {
               for (octave_idx_type i = 0; i < fn.numel (); i++)
                 {
-                  out.assign (fn (i), GetPermsNoSort<octave_value>
+                  out.assign (fn (i), GetPerms<octave_value>
                                       (map_in.contents (fn (i)), uniq_v));
                 }
             }