bugGNU Octave - Bugs: bug #51187, ISMEMBER fails if the string ends...

 
 

bug #51187: ISMEMBER fails if the string ends in a space:

Submitter:  deego <deego>
Submitted:  Tue 06 Jun 2017 05:58:54 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 14 Jun 2017 07:03:02 PM UTC, comment #16: 

Rik, thanks for checking.
Later on I'll come up with some doc fixes as regards char array processing by ismember - we cannot stay behind Matlab there can we.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 13 Jun 2017 04:51:48 PM UTC, comment #15: 

I checked the examples in comment #14 in Octave and they produce the same results as Matlab.  I think we're okay, and no need to add a 'legacy' flag.

Rik <rik5>
Group administrator
Tue 13 Jun 2017 04:46:15 PM UTC, comment #14: 

W/o reopening (yet), hereś an excerpt from an email thread I had with TMW tech support:

<QUOTE>
                                                      It is not specifically documented that for "ismember", character matrices the trailing spaces are ignored. Also, it is not documented that the behavior for character matrices is different than for character vectors. It should be though and this is one of the things I will be discussing with our documentation team.
</QUOTE>

So we get this:

>>format compact          % skip empty lines in output
>>ismember ('a ', {'a '})  % both arguments trailing space
ans =
  logical
   1
>> ismember ('a ', {'a'})   % only first arg has trailing space
ans =
  logical
   0
>> ismember ('a ', cellstr ('a '))   % both args trailing space, in 2nd arg truncated by ‘cellstr’
ans =
  logical
   0
>> ismember (['a '; 'b '], cellstr ('a '))   % arg #1 char array with trailing spaces
ans =
  2×1 logical array
   1
   0
>> size ('a ')
ans =
     1     2
>> size (['a '; 'b '])
ans =
     2     2


TMW (IMO correctly) mentioned that char arrays are actually a legacy type; one motive for truncating trailing spaces in char arrays by ismmber() is because there's no way to distinguish genuine input spaces from spaces originating from padding the char array to get rectangular.

I cannot check Octave's behavior now (busy bisecting s/th) but I think we'd better follow Matlab in truncating trailing spaces in char arrays, no matter how inconsistent.
Or, we could add a "legacy" flag (r2016b still has it) but as you mentioned that may be a bit far out for an obscure "bug" like this
If you want I could have a shot at it, though.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 10 Jun 2017 07:45:54 AM UTC, comment #13: 

Works fine on Linux and Windows.
Closing report

Philip Nienhuis <philipnienhuis>
Group Member
Fri 09 Jun 2017 06:32:09 PM UTC, comment #12: 

I agree, I dobut you'll get much satisfaction out of TMW's tech support.  In the meantime, I committed the simplest fix which still seems to resolve this issue.  See http://hg.savannah.gnu.org/hgweb/octave/rev/21b906769c0d.  I also added two BIST tests from this bug report.  Marking as "Ready for Test".

Rik <rik5>
Group administrator
Fri 09 Jun 2017 05:22:42 PM UTC, comment #11: 

In the mean time I entered a bug report on the TMW site as the change in treatment of char arrays by ismmber is nowhere documented.
The usual answer I get from TMW when filing this sort of bugs is that it is rather a "doumentation issue" (I interpret that they're too ignorant to fix bugs and just changing the docs is the least amount of work).
So I agree that we should copy the behavior of the latest Matlab release - chances that this is gonna be fixed can be considered slim.

Another thing (that I added in that bug report) is that the "legacy" flag already existed in r2012a, and there it served to relax type checking / acceptance of ismember input arguments.  So the purpose and results of that 'legacy' flag seem to have changed from release to release (.... no comment)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 09 Jun 2017 04:33:47 PM UTC, comment #10: 

It seems like the behavior is very complicated.  In the interest of minimizing the coding burden I would not support the 'legacy' flag and only support the most recent behavior, whatever it is.

As for cellstr and the trimming of spaces at the end, that is Matlab compatible behavior which needs to be kept.  But notice that the C++ code for cellstr uses an argument to the Cell constructor when it creates the Cell.


      string_vector s = args(0).xstring_vector_value ("cellstr: argument STRING must be a 2-D character array");

      return ovl (s.is_empty () ? Cell (octave_value (""))
                                : Cell (s, true));


The constructor for Cell has several variations.  The one in question is


  Cell (const string_vector& sv, bool trim = false);


One potential coding path would be to have a function, maybe one for internal use with '__' prefix and suffix, which calls Cell (sv, false) so that spaces are not trimmed.

Another option would be examine the code for lookup which is what ismember uses internally.  The relevant code is


  else if (str_case)
    {
      Array<std::string> str_table = table.cellstr_value ();
      Array<std::string> str_y (dim_vector (1, 1));

      if (y.is_cellstr ())
        str_y = y.cellstr_value ();
      else
        str_y(0) = y.string_value ();

      Array<octave_idx_type> idx = str_table.lookup (str_y);


Maybe cell_value rather than cellstr_value would be more appropriate for the initialization of str_table?

Or maybe the problem is with y.  In the m-file for either ismember or lookup single row char arrays could be converted to cells.  This seems to work with the examples given.


## I added { } around query to test single row char matrices being converted
ismember ({'b '}, {'a ', 'b '})
ans = 1
## multi-row char matrices are not converted, and give the same result as Matlab
abc = ['a '; 'b '; 'c ']
abc =

a
b
c

ismember (abc, {abc})
ans =

  0
  0
  0



A quick diff for that change is


diff -r 71bfd507663c scripts/set/ismember.m
--- a/scripts/set/ismember.m    Thu Jun 08 18:24:28 2017 -0700
+++ b/scripts/set/ismember.m    Fri Jun 09 09:31:32 2017 -0700
@@ -88,6 +88,10 @@ function [tf, s_idx] = ismember (a, s, v
     s = uint8 (s);
   endif

+  if (ischar (a) && rows (a) == 1 && iscell (s))
+    a = {a};
+  endif
+
   [a, s] = validsetargs ("ismember", a, s, varargin{:});

   by_rows = nargin == 3;


'test ismember' still passes which is a good thing, and the failing case also passes.

Rik <rik5>
Group administrator
Fri 09 Jun 2017 01:58:59 PM UTC, comment #9: 

OK I checked and it turns out that Matlab r2012a and r2016b behave differently as regards char arrays. For simple character strings (1D vectors) the behavior is as I wrote in comment #8:

R2016b
======

>> format compact
>> ismember ('b ', {'a ', 'b '})
ans =
  logical
   1
>> abc = ['a '; 'b '; 'c ']
abc =
a
b
c
>> ismember (abc, {abc})
ans =
  3×1 logical array
   0
   0
   0
>>


However with r2016b:

>> abc = ['a '; 'b '; 'c ']
abc =
a
b
c
>> ismember (abc, {abc}, 'legacy')
ans =
  3×1 logical array
   1
   1
   1
>>


R2012a
======

>> format compact
>> ismember ('b ', {'a ', 'b '})
ans =
     1
>>
>> abc = ['a '; 'b '; 'c ']
abc =
a
b
c
>> ismember (abc, {abc})
ans =
     1
     1
     1
>> ismember (abc, {'a '})
ans =
     1
     0
     0
>> ismember (abc, {'a ', 'b '})
ans =
     1
     1
     0
>> ismember (abc, {'a '; 'b '})
ans =
     1
     1
     0
>> ismember (abc, {['a '; 'b '; 'c ']})
ans =
     1
     1
     1
>>


The 'legacy' option seems to have been made especially for Tables and other new stuff like datetime arrays, duration arrays and timetables; although it existed already in r2012a where it apparently was meant to relax stricter rules regarding input types.
I wonder if treatment of 2D char arrays in recent matlab releases might have been an unintended "regression".
I didn't find info in Matlab's release notes, so I might enter a bug report for it in the Mathworks site.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 09 Jun 2017 09:28:14 AM UTC, comment #8: 

IIRC correctly (I already tried that this week) Matlab returns a single 1. In the online ML docs there's a similar example.
Later today I'll verify.

I attach a preliminary fix using a for loop (yes, ugly, but I couldn't find a vectorized way to convert a char array into a cellstr array w/o truncation trailing whitespace)

I have another case that needs attention and that I was busy trying to fix as well:

abc = ['a '; 'b '; 'c ']
ismember (abc, {abc})


Matlab (r2016b):

ans =
1
1
1


Octave, with the attached patch:

ans =
1
0
0



(file #40891)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 08 Jun 2017 09:08:24 PM UTC, comment #7: 

Could someone check what Matlab returns for


ismember ('b ', {'a ', 'b '})


I'm interested in knowing whether it returns a single value, or whether it treats the 'b' and ' ' separately so it returns [1 1].

Rik <rik5>
Group administrator
Wed 07 Jun 2017 08:02:56 PM UTC, comment #6: 

@Guillaume, comment #5:
In the inline Matlab docs (r2017a) for ismember there's a stanza that clearly indicates that Matlab's ismember honors trailing spaces.
If your first example came from Matlab, I think you've hit a Matlab bug (what ML version did you use then?)

Your suggestion for a fix won't work as it invokes cellstr() which is at the heart of the problem - that function strips the trailing spaces.

We need a way for turning a char array into a cellstring array while preserving trailing spaces.

Status -> in progress

Philip Nienhuis <philipnienhuis>
Group Member
Wed 07 Jun 2017 11:01:24 AM UTC, comment #5: 

When the input is a char array, it seems that it is transformed to a cell array in Matlab (with spaces stripped):


>> ismember(strvcat('a ','bc'),{'a ','bc'})

ans =

  2×1 logical array

   0
   1


so a fix could be:


if (size(x,1) == 1)
  x  = {x};
else
  x = cellstr (x);
endif


Guillaume <gyom>
Tue 06 Jun 2017 07:01:46 PM UTC, comment #4: 

Using cellstr and then adding back stripped spaces doesn't seem good to me.

I don't think you can simply substitute {} for cellstr because of the difference between


x = ['foo'; 'bar']
cellstr (x)  % two element cell array
{x}          % one element cell array


John W. Eaton <jwe>
Group administrator
Tue 06 Jun 2017 12:12:40 PM UTC, comment #3: 

The attached cset helps, but I'm unsure if it is the right fix.

Its effect is that it simply turns character arguments (#1 and/or #2) of ismember.m into a plain 1x1 cell array. AFAICS it also works with char arrays but I haven't tested it extensively.

The underlying problem is that cellstr() drops trailing spaces (BTW Matlab does the same), so we must somehow avoid cellstr() or implement a way to add back trailing spaces after a call to cellstr() (which will probably boil down to a kludge).


(file #40863)

Philip Nienhuis <philipnienhuis>
Group Member
Tue 06 Jun 2017 11:20:57 AM UTC, comment #2: 

BTW, trying with a simple space " " rather than "a " is sufficient

Philip Nienhuis <philipnienhuis>
Group Member
Tue 06 Jun 2017 11:19:31 AM UTC, comment #1: 

Compared with Matlab r2016b (example works fine there) and confirmed with dev version as well.

OS -> Any
Release -> dev

Philip Nienhuis <philipnienhuis>
Group Member
Tue 06 Jun 2017 05:58:54 AM UTC, original submission:  

octave:36> version
ans = 4.0.3 ## installed via backports.


octave:37> a = "a "; ismember(a,{a})
ans = 0


.. on Debian 8 jessie stable.

deego <deego>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #40891:  ismember_bug51178_#2.cset added by philipnienhuis (2KiB - application/octet-stream)
file #40863:  bug51187_ismember.cset added by philipnienhuis (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by deego (Submitted the item)
  • -email is unavailable- added by deego
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-10 philipnienhuis StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-06-09 rik5 StatusIn Progress Ready For Test
    2017-06-09 philipnienhuis Attached File- Added ismember_bug51178_#2.cset, #40891
    2017-06-07 philipnienhuis StatusPatch Submitted In Progress
    2017-06-06 philipnienhuis Attached File- Added bug51187_ismember.cset, #40863
        StatusConfirmed Patch Submitted
    2017-06-06 philipnienhuis StatusNone Confirmed
        Release4.0.3 dev
        Operating SystemGNU/Linux Any
    2017-06-06 deego Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code