changeset 32058:e242124f1240

Overhaul input validation of sparse() function. * sparse.cc (Fsparse): Decode input type and identify floating point inputs. If input is of single type, emit new warning "Octave:sparse:double-conversion". If input is neither floating point or logical, then call err_wrong_type_arg() for pretty error message. Rename temporary variable 'k' to "argidx" for clarity. New temporary variable "arg" to increase readability of code. Add FIXME note about unreachable code due to behavior of get_dimensions(). * sparse.cc (Fissparse): Turn off warning about double-conversion temporarily for test which has single input. * warning_ids.m: Add description for new warning ID "Octave:sparse:double-conversion". * mk-sparse-tst.sh: Redo BIST tests for sparse() construction.
author Rik <rik@octave.org>
date Wed, 26 Apr 2023 10:09:09 -0700
parents f010a32986e4
children bade9602c5a1
files libinterp/corefcn/sparse.cc scripts/help/warning_ids.m test/mk-sparse-tst.sh
diffstat 3 files changed, 80 insertions(+), 30 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/sparse.cc	Wed Apr 26 09:55:12 2023 -0700
+++ b/libinterp/corefcn/sparse.cc	Wed Apr 26 10:09:09 2023 -0700
@@ -62,7 +62,9 @@
 %!assert (issparse (1), false)
 %!assert (issparse (sparse (false)), true)
 %!assert (issparse (true), false)
-%!assert (issparse (sparse (single ([1 2]))), true)
+%!test
+%! warning ("off", "Octave:sparse:double-conversion", "local");
+%! assert (issparse (sparse (single ([1 2]))), true);
 %!assert (issparse (single ([1, 2])), false)
 %!assert (issparse (sparse ([1+i, 2]')), true)
 %!assert (issparse ([1+i, 2]'), false)
@@ -81,10 +83,10 @@
 DEFUN (sparse, args, ,
        doc: /* -*- texinfo -*-
 @deftypefn  {} {@var{S} =} sparse (@var{A})
-@deftypefnx {} {@var{S} =} sparse (@var{i}, @var{j}, @var{sv}, @var{m}, @var{n})
+@deftypefnx {} {@var{S} =} sparse (@var{m}, @var{n})
 @deftypefnx {} {@var{S} =} sparse (@var{i}, @var{j}, @var{sv})
-@deftypefnx {} {@var{S} =} sparse (@var{m}, @var{n})
-@deftypefnx {} {@var{S} =} sparse (@var{i}, @var{j}, @var{s}, @var{m}, @var{n}, "unique")
+@deftypefnx {} {@var{S} =} sparse (@var{i}, @var{j}, @var{sv}, @var{m}, @var{n})
+@deftypefnx {} {@var{S} =} sparse (@var{i}, @var{j}, @var{sv}, @var{m}, @var{n}, "unique")
 @deftypefnx {} {@var{S} =} sparse (@var{i}, @var{j}, @var{sv}, @var{m}, @var{n}, @var{nzmax})
 Create a sparse matrix from a full matrix @var{A} or row, column, value
 triplets.
@@ -93,6 +95,9 @@
 removing all zero values in the process.  The matrix @var{A} should be of type
 logical or double.
 
+If two inputs @var{m} (rows) and @var{n} (columns) are specified then create
+an empty sparse matrix with the specified dimensions.
+
 Given the integer index vectors @var{i} and @var{j}, and a 1-by-@code{nnz}
 vector of real or complex values @var{sv}, construct the sparse matrix
 @code{S(@var{i}(@var{k}),@var{j}(@var{k})) = @var{sv}(@var{k})} with overall
@@ -111,7 +116,8 @@
 
 If the option @qcode{"unique"} is given, and more than one value is specified
 at the same @var{i}, @var{j} indices, then only the last specified value will
-be used.
+be used.  For completeness, the option @qcode{"sum"} can be given and will
+be ignored as the default behavior is to sum values at repeated locations.
 
 @code{sparse (@var{m}, @var{n})} will create an empty @var{m}x@var{n} sparse
 matrix and is equivalent to @code{sparse ([], [], [], @var{m}, @var{n})}
@@ -176,12 +182,18 @@
   if (nargin == 1)
     {
       octave_value arg = args(0);
-      if (arg.islogical ())
+      if (arg.isfloat ())
+        {
+          if (arg.is_single_type ())
+            warning_with_id ("Octave:sparse:double-conversion",
+                             "sparse: input array cast to double");
+          if (arg.iscomplex ())
+            retval = arg.sparse_complex_matrix_value ();
+          else
+            retval = arg.sparse_matrix_value ();
+        }
+      else if (arg.islogical ())
         retval = arg.sparse_bool_matrix_value ();
-      else if (arg.iscomplex ())
-        retval = arg.sparse_complex_matrix_value ();
-      else if (arg.isnumeric ())
-        retval = arg.sparse_matrix_value ();
       else
         err_wrong_type_arg ("sparse", arg);
     }
@@ -192,6 +204,8 @@
 
       get_dimensions (args(0), args(1), "sparse", m, n);
 
+      // FIXME: this code is never active because get_dimensions()
+      //        replaces negative dimensions with 0.
       if (m < 0 || n < 0)
         error ("sparse: dimensions must be non-negative");
 
@@ -225,33 +239,42 @@
         {
           get_dimensions (args(3), args(4), "sparse", m, n);
 
+          // FIXME: this code is never active because get_dimensions()
+          //        replaces negative dimensions with 0.
           if (m < 0 || n < 0)
             error ("sparse: dimensions must be non-negative");
         }
 
-      int k = 0;    // index we're checking when index_vector throws
+      int argidx = 0;    // index we're checking when index_vector throws
       try
         {
           idx_vector i = args(0).index_vector ();
-          k = 1;
+          argidx = 1;
           idx_vector j = args(1).index_vector ();
 
-          if (args(2).islogical ())
-            retval = SparseBoolMatrix (args(2).bool_array_value (), i, j,
-                                       m, n, summation, nzmax);
-          else if (args(2).iscomplex ())
-            retval = SparseComplexMatrix (args(2).complex_array_value(),
-                                          i, j, m, n, summation, nzmax);
-          else if (args(2).isnumeric ())
-            retval = SparseMatrix (args(2).array_value (), i, j,
-                                   m, n, summation, nzmax);
+          octave_value arg = args(2);  // temp var for code readability
+          if (arg.isfloat ())
+            {
+              if (arg.is_single_type ())
+                warning_with_id ("Octave:sparse:double-conversion",
+                                 "sparse: input array cast to double");
+              if (arg.iscomplex ())
+                retval = SparseComplexMatrix (arg.complex_array_value (),
+                                              i, j, m, n, summation, nzmax);
+              else
+                retval = SparseMatrix (arg.array_value (),
+                                       i, j, m, n, summation, nzmax);
+            }
+          else if (arg.islogical ())
+            retval = SparseBoolMatrix (arg.bool_array_value (),
+                                       i, j, m, n, summation, nzmax);
           else
-            err_wrong_type_arg ("sparse", args(2));
+            err_wrong_type_arg ("sparse", arg);
         }
       catch (index_exception& ie)
         {
           // Rethrow to allow more info to be reported later.
-          ie.set_pos_if_unset (2, k+1);
+          ie.set_pos_if_unset (2, argidx+1);
           throw;
         }
     }
--- a/scripts/help/warning_ids.m	Wed Apr 26 09:55:12 2023 -0700
+++ b/scripts/help/warning_ids.m	Wed Apr 26 10:09:09 2023 -0700
@@ -410,6 +410,12 @@
 ## string constant.
 ## By default, the @code{Octave:single-quote-string} warning is disabled.
 ##
+## @item Octave:sparse:double-conversion
+## If the @code{Octave:sparse:double-conversion} warning is enabled, a warning
+## is printed when an implicit conversion from a full, single array occurs
+## during the creation of a sparse array.
+## By default, the @code{Octave:sparse:double-conversion} warning is enabled.
+##
 ## @item Octave:sqrtm:SingularMatrix
 ## If the @code{Octave:sqrtm:SingularMatrix} warning is enabled, a warning is
 ## printed if the matrix square root function @code{sqrtm} is called with an
@@ -459,5 +465,5 @@
 endfunction
 
 
-## Mark file as tested.  No test needed for a documentation m-file.
+## Mark file as tested.  No tests needed for a documentation m-file.
 %!assert (1)
--- a/test/mk-sparse-tst.sh	Wed Apr 26 09:55:12 2023 -0700
+++ b/test/mk-sparse-tst.sh	Wed Apr 26 10:09:09 2023 -0700
@@ -174,6 +174,33 @@
 gen_specific() {
 cat <<EOF
 
+%% error handling in constructor
+%!error <Invalid call> sparse ()
+%!error <Invalid call> sparse (1,2,3,4,5,6,7)
+%!warning <input array cast to double>
+%! warning ("on", "Octave:sparse:double-conversion", "local");
+%! s = sparse (single ([1 2]));
+%!error <wrong type argument 'uint8 matrix'>
+%! s = sparse (uint8 ([1 2]));
+%% FIXME: negative dimensions are allowed and replaced with 0.
+%%        If this is fixed, re-instate these tests.
+%!#error <dimensions must be non-negative> sparse (-1, 2)
+%!#error <dimensions must be non-negative> sparse (1, -2)
+%!error <dimension mismatch> sparse (1,[2,3],[1,2,3])
+%!error <invalid option: foobar> sparse ([1,1],[1,1],[1,2],"foobar")
+%% negative subscripts are disallowed
+%!error <subscripts must be> sparse ([1,3],[1,-4],[3,5],2,2)
+%!error <subscripts must be> sparse ([1,3],[1,-4],[3,5i],2,2)
+%% FIXME: negative dimensions are allowed and replaced with 0.
+%%        If this is fixed, re-instate these tests.
+%!#error <dimensions must be non-negative> sparse ([1,1],[1,1],[1,2], -1, 2)
+%!#error <dimensions must be non-negative> sparse ([1,1],[1,1],[1,2], 1, -2)
+%!warning <input array cast to double>
+%! warning ("on", "Octave:sparse:double-conversion", "local");
+%! s = sparse ([1,1],[1,1], single ([1,2]), 2, 2);
+%!error <wrong type argument 'uint8 matrix'>
+%! s = sparse ([1,1],[1,1], uint8 ([1,2]), 2, 2);
+
 %!test # segfault test from edd@debian.org
 %! n = 510;
 %! sparse (kron ((1:n)', ones (n,1)), kron (ones (n,1), (1:n)'), ones (n));
@@ -189,12 +216,6 @@
 %#!error inv ( sparse ([0,0;0,1+i]) );
 %#!error inv ( sparse ([0,0;0,0]  ) );
 
-%% error handling in constructor
-%!error sparse (1,[2,3],[1,2,3])
-%!error sparse ([1,1],[1,1],[1,2],3,3,"invalid")
-%!error sparse ([1,3],[1,-4],[3,5],2,2)
-%!error sparse ([1,3],[1,-4],[3,5i],2,2)
-%!error sparse (-1,-1,1)
 EOF
 }