Error with pkg.m (== isspace does work with cell arrays anymore)

[Michael Goffioul-2]

I have an error with recent mercurial code, in pkg.m. It reduces to
the fact that isspace does not work anymore on cell array of
strings. The error occurs around line 1299:

  if (! all (isspace (filenames)))

where filenames is a cell array of strings. I'm not sure what's
the correct way to fix this.

Michael.

[John W. Eaton]

On 24-Feb-2008, Michael Goffioul wrote:

| I have an error with recent mercurial code, in pkg.m. It reduces to
| the fact that isspace does not work anymore on cell array of
| strings. The error occurs around line 1299:
|
|   if (! all (isspace (filenames)))
|
| where filenames is a cell array of strings. I'm not sure what's
| the correct way to fix this.

I could probably help if I knew what isspace should do for a cell
array.  Matlab appears to do this:

  >> isspace ({' ', ' ', ' '})      

  ans =

       0     0     0


Which makes no sense to me.  Is this behavior useful in some way that
I'm not seeing?

jwe

[Michael Goffioul-2]

On 2/25/08, John W. Eaton <jwe@...> wrote:

> I could probably help if I knew what isspace should do for a cell
> array.  Matlab appears to do this:
>
>  >> isspace ({' ', ' ', ' '})
>
>  ans =
>
>       0     0     0
>
>
> Which makes no sense to me.  Is this behavior useful in some way that
> I'm not seeing?

I don't know what's the expected behavior of isspace calls in
pkg.m. Maybe David has a better understanding of this.

Michael.

[Daniel J Sebald]

John W. Eaton wrote:

> On 24-Feb-2008, Michael Goffioul wrote:
>
> | I have an error with recent mercurial code, in pkg.m. It reduces to
> | the fact that isspace does not work anymore on cell array of
> | strings. The error occurs around line 1299:
> |
> |   if (! all (isspace (filenames)))
> |
> | where filenames is a cell array of strings. I'm not sure what's
> | the correct way to fix this.
>
> I could probably help if I knew what isspace should do for a cell
> array.  Matlab appears to do this:
>
>   >> isspace ({' ', ' ', ' '})      
>
>   ans =
>
>        0     0     0
>
>
> Which makes no sense to me.  Is this behavior useful in some way that
> I'm not seeing?

Well, in that case the question is whether the cell is a space, which I suppose it isn't.  Consider:

octave:14> ['left' ' ' 'right']
ans = left right
octave:15> ['left' {' '} 'right']
error: concatenation operator not implemented for `sq_string' by `cell' operations
octave:15> ['left' {' '}{} 'right']
ans = left right

Dan

[Søren Hauberg]

man, 25 02 2008 kl. 16:17 -0500, skrev John W. Eaton:

> On 24-Feb-2008, Michael Goffioul wrote:
>
> | I have an error with recent mercurial code, in pkg.m. It reduces to
> | the fact that isspace does not work anymore on cell array of
> | strings. The error occurs around line 1299:
> |
> |   if (! all (isspace (filenames)))
> |
> | where filenames is a cell array of strings. I'm not sure what's
> | the correct way to fix this.
>
> I could probably help if I knew what isspace should do for a cell
> array.  Matlab appears to do this:
>
>   >> isspace ({' ', ' ', ' '})      
>
>   ans =
>
>        0     0     0
>
>
> Which makes no sense to me.  Is this behavior useful in some way that
> I'm not seeing?

It seems to be that matlab is doing something like

function output = isspace(input)
  if (ischar(input))
    ...
  else
    output = zeros(size(input), "logical");
  endif
endfunction

As an example:
  isspace(@sin)
returns 0. Does that make sense? Not in my mind. I think the logic is:

  "A space is part of a string, hence if the input is not a string, it
is not a space."

I don't think we should copy this. If people depend on this behaviour,
then there's a pretty good chance of a bug in their code.

Søren

[John W. Eaton]

On 25-Feb-2008, Søren Hauberg wrote:

|
| man, 25 02 2008 kl. 16:17 -0500, skrev John W. Eaton:
| > On 24-Feb-2008, Michael Goffioul wrote:
| >
| > | I have an error with recent mercurial code, in pkg.m. It reduces to
| > | the fact that isspace does not work anymore on cell array of
| > | strings. The error occurs around line 1299:
| > |
| > |   if (! all (isspace (filenames)))
| > |
| > | where filenames is a cell array of strings. I'm not sure what's
| > | the correct way to fix this.
| >
| > I could probably help if I knew what isspace should do for a cell
| > array.  Matlab appears to do this:
| >
| >   >> isspace ({' ', ' ', ' '})      
| >
| >   ans =
| >
| >        0     0     0
| >
| >
| > Which makes no sense to me.  Is this behavior useful in some way that
| > I'm not seeing?
| It seems to be that matlab is doing something like
|
| function output = isspace(input)
|   if (ischar(input))
|     ...
|   else
|     output = zeros(size(input), "logical");
|   endif
| endfunction
|
| As an example:
|   isspace(@sin)
| returns 0. Does that make sense? Not in my mind. I think the logic is:
|
|   "A space is part of a string, hence if the input is not a string, it
| is not a space."

Yes, probably an error is better.

| I don't think we should copy this. If people depend on this behaviour,
| then there's a pretty good chance of a bug in their code.

OK, isspace appears to be unique.  Matlab doesn't seem to have the
other is* predicates, but it does have isstrprop, which lumps them all
in one function.  I propose the following change.  The added functions in
{Cell,ov-cell}.{h,cc} make writing the isstrprop function fairly
simple.

I would guess this still doesn't solve the isspace problem in pkg.m.
We need to know what the intent of the isspace call there is.

jwe



# HG changeset patch
# User John W. Eaton <jwe@...>
# Date 1203984090 18000
# Node ID bb0f2353cff56ce32d23919fc684174158826009
# Parent  7e1b042c5418240ec84cc3bf46e1f9d91d060105
new cell array ctype mappers

diff --git a/scripts/ChangeLog b/scripts/ChangeLog
--- a/scripts/ChangeLog
+++ b/scripts/ChangeLog
@@ -1,3 +1,7 @@ 2008-02-25  Ryan Hinton  <rwh4s@virginia
[phone]  John W. Eaton  <jwe@...>
+
+ * strings/isstrprop.m: New file.
+
 2008-02-25  Ryan Hinton  <rwh4s@...>
 
  * miscellaneous/unpack.m: Use "-f -" args for tar.
diff --git a/scripts/strings/isstrprop.m b/scripts/strings/isstrprop.m
new file mode 100644
--- /dev/null
+++ b/scripts/strings/isstrprop.m
@@ -0,0 +1,118 @@
+## Copyright (C) 2008 John W. Eaton
+##
+## This file is part of Octave.
+##
+## Octave is free software; you can redistribute it and/or modify it
+## under the terms of the GNU General Public License as published by
+## the Free Software Foundation; either version 3 of the License, or (at
+## your option) any later version.
+##
+## Octave is distributed in the hope that it will be useful, but
+## WITHOUT ANY WARRANTY; without even the implied warranty of
+## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+## General Public License for more details.
+##
+## You should have received a copy of the GNU General Public License
+## along with Octave; see the file COPYING.  If not, see
+## <http://www.gnu.org/licenses/>.
+
+## -*- texinfo -*-
+## @deftypefn {Function File} {} isstrprop (@var{str}, @var{pred})
+## Test character string properties.  For example,
+##
+## @example
+## @group
+## isstrprop ("abc123", "isalpha")
+## @result{} [1, 1, 1, 0, 0, 0]
+## @end group
+## @end example
+##
+## If @var{str} is a cell array, @code{isstrpop} is applied recursively
+## to each element of the cell array.
+##
+## Numeric arrays are converted to character strings.
+##
+## The second argument @var{pred} may be one of
+##
+## @table @code
+## @item "alpha"
+## True for characters that are alphabetic
+##
+## @item "alnum"
+## @itemx "alphanum"
+## True for characters that are alphabetic or digits.
+##
+## @item "ascii"
+## True for characters that are in the range of ASCII encoding.
+##
+## @item "cntrl"
+## True for control characters.
+##
+## @item "digit"
+## True for decimal digits.
+##
+## @item "graph"
+## @itemx "graphic"
+## True for printing characters except space.
+##
+## @item "lower"
+## True for lower-case letters.
+##
+## @item "print"
+## True for printing characters including space.
+##
+## @item "punct"
+## True for printing characters except space or letter or digit.
+##
+## @item "space"
+## @itemx "wspace"
+## True for whitespace characters (space, formfeed, newline, carriage
+## return, tab, vertical tab).
+##
+## @item "upper"
+## True for upper-case letters.
+##
+## @item "xdigit"
+## True for hexadecimal digits.
+## @end table
+##
+## @seealso{isalnum, isalpha, isascii, iscntrl, isdigit, isgraph,
+## islower, isprint, ispunct, isspace, isupper, isxdigit}
+## @end deftypefn
+
+function retval = isstrprop (str, pred)
+
+  if (nargin == 2)
+    switch (pred)
+      case "alpha"
+ retval = isalpha (str);
+      case {"alnum", "alphanum"}
+ retval = isalnum (str);
+      case "ascii"
+ retval = isascii (str);
+      case "cntrl"
+ retval = iscntrl (str);
+      case "digit"
+ retval = isdigit (str);
+      case {"graph", "graphic"}
+ retval = isgraph (str);
+      case "lower"
+ retval = islower (str);
+      case "print"
+ retval = isprint (str);
+      case "punct"
+ retval = ispunct (str);
+      case {"space", "wspace"}
+ retval = isspace (str);
+      case "upper"
+ retval = isupper (str);
+      case "xdigit"
+ retval = isxdigit (str);
+      otherwise
+ error ("isstrprop: invalid predicate");
+    endswitch
+  else
+    print_usage ();
+  endif
+
+endfunction
diff --git a/src/Cell.cc b/src/Cell.cc
--- a/src/Cell.cc
+++ b/src/Cell.cc
@@ -224,6 +224,20 @@ Cell::insert (const Cell& a, const Array
   return *this;
 }
 
+Cell
+Cell::map (ctype_mapper fcn) const
+{
+  Cell retval (dims ());
+  octave_value *r = retval.fortran_vec ();
+
+  const octave_value *p = data ();
+
+  for (octave_idx_type i = 0; i < numel (); i++)
+    r[i] = ((p++)->*fcn) ();
+
+  return retval;
+}
+
 /*
 ;;; Local Variables: ***
 ;;; mode: C++ ***
diff --git a/src/Cell.h b/src/Cell.h
--- a/src/Cell.h
+++ b/src/Cell.h
@@ -114,6 +114,28 @@ public:
   bool is_true (void) const { return false; }
 
   static octave_value resize_fill_value (void) { return Matrix (); }
+
+  Cell xisalnum (void) const { return map (&octave_value::xisalnum); }
+  Cell xisalpha (void) const { return map (&octave_value::xisalpha); }
+  Cell xisascii (void) const { return map (&octave_value::xisascii); }
+  Cell xiscntrl (void) const { return map (&octave_value::xiscntrl); }
+  Cell xisdigit (void) const { return map (&octave_value::xisdigit); }
+  Cell xisgraph (void) const { return map (&octave_value::xisgraph); }
+  Cell xislower (void) const { return map (&octave_value::xislower); }
+  Cell xisprint (void) const { return map (&octave_value::xisprint); }
+  Cell xispunct (void) const { return map (&octave_value::xispunct); }
+  Cell xisspace (void) const { return map (&octave_value::xisspace); }
+  Cell xisupper (void) const { return map (&octave_value::xisupper); }
+  Cell xisxdigit (void) const { return map (&octave_value::xisxdigit); }
+  Cell xtoascii (void) const { return map (&octave_value::xtoascii); }
+  Cell xtolower (void) const { return map (&octave_value::xtolower); }
+  Cell xtoupper (void) const { return map (&octave_value::xtoupper); }
+
+private:
+
+  typedef octave_value (octave_value::*ctype_mapper) (void) const;
+
+  Cell map (ctype_mapper) const;
 };
 
 #endif
diff --git a/src/ChangeLog b/src/ChangeLog
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,16 @@ 2008-02-25  Michael Goffioul  <michael.g
[phone]  John W. Eaton  <jwe@...>
+
+ * Cell.cc (Cell::map): New function.
+ * Cell.h (Cell::map): Declare.
+ (xisalnum, xisalpha, xisascii, xiscntrl, xisdigit,
+ xisgraph, xislower, xisprint, xispunct, xisspace, xisupper,
+ xisxdigit, xtoascii, xtolower, xtoupper): New mapper functions.
+ (ctype_mapper): New private typedef.
+
+ * ov-cell.h (xisalnum, xisalpha, xisascii, xiscntrl, xisdigit,
+ xisgraph, xislower, xisprint, xispunct, xisspace, xisupper,
+ xisxdigit, xtoascii, xtolower, xtoupper): New mapper functions.
+
 2008-02-25  Michael Goffioul  <michael.goffioul@...>
 
  * ov-scalar.cc (octave_scalar::round): Use xround instead of ::round
diff --git a/src/ov-cell.h b/src/ov-cell.h
--- a/src/ov-cell.h
+++ b/src/ov-cell.h
@@ -130,6 +130,22 @@ public:
   bool load_hdf5 (hid_t loc_id, const char *name, bool have_h5giterate_bug);
 #endif
 
+  octave_value xisalnum (void) const { return matrix.xisalnum (); }
+  octave_value xisalpha (void) const { return matrix.xisalpha (); }
+  octave_value xisascii (void) const { return matrix.xisascii (); }
+  octave_value xiscntrl (void) const { return matrix.xiscntrl (); }
+  octave_value xisdigit (void) const { return matrix.xisdigit (); }
+  octave_value xisgraph (void) const { return matrix.xisgraph (); }
+  octave_value xislower (void) const { return matrix.xislower (); }
+  octave_value xisprint (void) const { return matrix.xisprint (); }
+  octave_value xispunct (void) const { return matrix.xispunct (); }
+  octave_value xisspace (void) const { return matrix.xisspace (); }
+  octave_value xisupper (void) const { return matrix.xisupper (); }
+  octave_value xisxdigit (void) const { return matrix.xisxdigit (); }
+  octave_value xtoascii (void) const { return matrix.xtoascii (); }
+  octave_value xtolower (void) const { return matrix.xtolower (); }
+  octave_value xtoupper (void) const { return matrix.xtoupper (); }
+
   mxArray *as_mxArray (void) const;
 
 private:

[Daniel J Sebald]

John W. Eaton wrote:

> On 25-Feb-2008, Søren Hauberg wrote:
>
> |
> | man, 25 02 2008 kl. 16:17 -0500, skrev John W. Eaton:
> | > On 24-Feb-2008, Michael Goffioul wrote:
> | >
> | > | I have an error with recent mercurial code, in pkg.m. It reduces to
> | > | the fact that isspace does not work anymore on cell array of
> | > | strings. The error occurs around line 1299:
> | > |
> | > |   if (! all (isspace (filenames)))
> | > |
> | > | where filenames is a cell array of strings. I'm not sure what's
> | > | the correct way to fix this.
> | >
> | > I could probably help if I knew what isspace should do for a cell
> | > array.  Matlab appears to do this:
> | >
> | >   >> isspace ({' ', ' ', ' '})      
> | >
> | >   ans =
> | >
> | >        0     0     0
> | >
> | >
> | > Which makes no sense to me.  Is this behavior useful in some way that
> | > I'm not seeing?
> | It seems to be that matlab is doing something like
> |
> | function output = isspace(input)
> |   if (ischar(input))
> |     ...
> |   else
> |     output = zeros(size(input), "logical");
> |   endif
> | endfunction
> |
> | As an example:
> |   isspace(@sin)
> | returns 0. Does that make sense? Not in my mind. I think the logic is:
> |
> |   "A space is part of a string, hence if the input is not a string, it
> | is not a space."
>
> Yes, probably an error is better.
>
> | I don't think we should copy this. If people depend on this behaviour,
> | then there's a pretty good chance of a bug in their code.
>
> OK, isspace appears to be unique.  Matlab doesn't seem to have the
> other is* predicates, but it does have isstrprop, which lumps them all
> in one function.  I propose the following change.  The added functions in
> {Cell,ov-cell}.{h,cc} make writing the isstrprop function fairly
> simple.
>
> I would guess this still doesn't solve the isspace problem in pkg.m.
> We need to know what the intent of the isspace call there is.

I would think that conditional test could be rewritten.  filenames is a cell of strings.  It wouldn't be difficult to write a short bit of code to test every member of the cell.  One wonders if there were a way to do that with a simple change of syntax.  This makes sense:

octave:25> x = [1 2; 3 4]
x =

   1   2
   3   4

octave:26> x(:)
ans =

   1
   3
   2
   4

and this makes sense:

octave:27> filenames = {'this' 'is' 'a' 'test'}
filenames =

{
  [1,1] = this
  [1,2] = is
  [1,3] = a
  [1,4] = test
}

octave:28> filenames{:}
ans =

(,
  [1] = this
  [2] = is
  [3] = a
  [4] = test
,)

And the following makes sense:

octave:30> filenames{1}(:)
ans =

t
h
i
s

so why couldn't we do:

octave:34> filenames{:}(:)
error: can't perform indexing operations for cs-list type
octave:34>

in the instance where every element of the list is a character?

Dan

[John W. Eaton]

On 25-Feb-2008, Daniel J Sebald wrote:

| so why couldn't we do:
|
| octave:34> filenames{:}(:)
| error: can't perform indexing operations for cs-list type
| octave:34>
|
| in the instance where every element of the list is a character?

Because a comma separated list (the result of something like
filenames{:}) is not something that can be assigned a single variable.
Think of it as an argument list, not a single object.

jwe

[Daniel J Sebald]

John W. Eaton wrote:

> On 25-Feb-2008, Daniel J Sebald wrote:
>
> | so why couldn't we do:
> |
> | octave:34> filenames{:}(:)
> | error: can't perform indexing operations for cs-list type
> | octave:34>
> |
> | in the instance where every element of the list is a character?
>
> Because a comma separated list (the result of something like
> filenames{:}) is not something that can be assigned a single variable.
> Think of it as an argument list, not a single object.

Oh, OK... Wait, got it.  Try this syntax:

octave:1> filenames = {'this' 'is' 'a' 'test'}
filenames =

{
  [1,1] = this
  [1,2] = is
  [1,3] = a
  [1,4] = test
}

octave:2> [filenames{}]
ans = thisisatest
octave:3> isspace([filenames{}])
ans =

   0   0   0   0   0   0   0   0   0   0   0

octave:4> filenames2 = {' ' ' ' ' ' ' '}
filenames2 =

{
  [1,1] =
  [1,2] =
  [1,3] =
  [1,4] =
}

octave:5> isspace([filenames2{}])
ans =

   1   1   1   1

octave:6>


It's actually kind of nice it works that way.  Does that work for you Michael?  Replacing all "filenames" by "[filenames{}]" in pkg.m?

Dan

[Daniel J Sebald]

Daniel J Sebald wrote:

> octave:5> isspace([filenames2{}])
> ans =
>
>   1   1   1   1
>
> octave:6>

Don't even need the braces:

octave:10> isspace([filenames2])
ans =

   1
   1
   1
   1

[Michael Goffioul-2]

Please consider the attach patch to make pkg.m work again with
the recent changes in isspace for cell arrays of strings.

Michael.

# HG changeset patch
# User Michael Goffioul <michael.goffioul@...>
# Date 1204022109 -3600
# Node ID 19f66eb9d3b0668609d86b1ff8e3cd4f7b61e1a6
# Parent  a816ee3114ccbf56354b38bcd72f29e75ee8ff0f
pkg.m: adapt to changes in isspace for cell arrays of strings.

diff -r a816ee3114cc -r 19f66eb9d3b0 scripts/ChangeLog
--- a/scripts/ChangeLog Tue Feb 26 10:09:39 2008 +0100
+++ b/scripts/ChangeLog Tue Feb 26 11:35:09 2008 +0100
@@ -1,3 +1,8 @@
[phone]  Michael Goffioul <michael.goffioul@...>
+
+ * pkg/pkg.m (pkg:configure_make): Make it work with recent changes in
+ isspace handling with cell arrays of strings.
+
 2008-02-25  John W. Eaton  <jwe@...>
 
  * strings/isstrprop.m: New file.
diff -r a816ee3114cc -r 19f66eb9d3b0 scripts/pkg/pkg.m
--- a/scripts/pkg/pkg.m Tue Feb 26 10:09:39 2008 +0100
+++ b/scripts/pkg/pkg.m Tue Feb 26 11:35:09 2008 +0100
@@ -1296,11 +1296,11 @@
     archindependent = filenames (!idx);
 
     ## Copy the files
-    if (! all (isspace (filenames)))
+    if (! all (isspace ([filenames{:}])))
  if (! exist (instdir, "dir"))
   mkdir (instdir);
  endif
- if (! all (isspace (archindependent)))
+ if (! all (isspace ([archindependent{:}])))
   if (verbose)
     printf ("copyfile");
     printf (" %s", archindependent{:});
@@ -1312,7 +1312,7 @@
     error ("Couldn't copy files from 'src' to 'inst': %s", output);
   endif
         endif
- if (! all (isspace (archdependent)))
+ if (! all (isspace ([archdependent{:}])))
   if (verbose)
     printf ("copyfile");
     printf (" %s", archdependent{:});

[dbateman]

Michael Goffioul-2 wrote:

Please consider the attach patch to make pkg.m work again with
the recent changes in isspace for cell arrays of strings.

Not having access to a copy of Octave at the moment, I can't tell if this is right. However if it works for you it probably is ...

D.

[John W. Eaton]

On 26-Feb-2008, Michael Goffioul wrote:

| Please consider the attach patch to make pkg.m work again with
| the recent changes in isspace for cell arrays of strings.

I applied this change.

Thanks,

jwe