bugGNU Octave - Bugs: bug #52479, textscan ignores leading spaces...

 
 

bug #52479: textscan ignores leading spaces when creating cell from string/file

Submitter:  Motius <motius>
Submitted:  Wed 22 Nov 2017 01:11:33 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Motius Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 29 Nov 2017 05:08:34 AM UTC, comment #13: 

I opened a new bug report for the observation in comment 12:

https://savannah.gnu.org/bugs/index.php?52550

because this seems different enough from the original report and is not related to white-space.  Consider that similar behavior occurs when the delimiter is a comma:


octave:1> a = ",,a,b,c\n";
octave:2> textscan(a, '%s', 'delimiter', ',')
ans =
{
  [1,1] =
  {
    [1,1] =
    [2,1] =
    [3,1] = a
    [4,1] = b
    [5,1] = c
  }

}

octave:3> textscan(a, '%s', 'delimiter', {','})
ans =
{
  [1,1] =
  {
    [1,1] =
    [2,1] = a
    [3,1] = b
    [4,1] = c

  }

}


Let's keep this report concise and not conflate it with the differences between string-specified and cell-specified delimiters.  I'll address various things, including compatability at the new bug report.  We can close this one.

Dan Sebald <sebald>
Sat 25 Nov 2017 01:34:58 AM UTC, comment #12: 

I checked in the change on stable here (http://hg.savannah.gnu.org/hgweb/octave/rev/ddc91a2ee0e0).  That still leaves a problem when the delimiter is specified in a cell array, rather than as a single character.  When specified as a single character, the code works.


octave:1> a = "\t\ta\tb\tc\n";
octave:2> textscan(a, '%s', 'delimiter', sprintf('\t'))
ans =
{
  [1,1] =
  {
    [1,1] =
    [2,1] =
    [3,1] = a
    [4,1] = b
    [5,1] = c
  }

}


But when the same character is presented as part of a cell array it is one entry too short.


octave:4> textscan(a, '%s', 'delimiter', {sprintf('\t')})
ans =
{
  [1,1] =
  {
    [1,1] =
    [2,1] = a
    [3,1] = b
    [4,1] = c

  }

}


Someone want to take a look at that?  It is no longer a regression, so I'm changing the report status to "Matlab Compatibility".


Rik <rik5>
Group administrator
Fri 24 Nov 2017 07:50:01 PM UTC, comment #11: 

Updated patch looks nice.  I imagined there must be a C++ routine that removes a letter from a string, but I could find no such function.  I wasn't aware of the iterator-based <algorithm>.

Dan Sebald <sebald>
Fri 24 Nov 2017 10:37:38 AM UTC, comment #10: 


a = sprintf ('\t\ta\tb\tc\n');
textscan(a, '%s', 'delimiter', {sprintf('\t')})


yields

ans =

 cell

   {5×1 cell}


with Matlab R2017a (windows)

Motius <motius>
Fri 24 Nov 2017 10:05:44 AM UTC, comment #9: 

Sorry, I forget that Matlab has introduced string arrays that use double quotes as a creation symbol.  I meant to create a string and process that. 


a = sprintf ('\t\ta\tb\tc\n');
textscan(a, '%s', 'delimiter', {sprintf('\t')})



Rik <rik5>
Group administrator
Fri 24 Nov 2017 09:28:02 AM UTC, comment #8: 

Hello,

results of:

a = "\t\ta\tb\tc\n"
textscan(a, '%s', 'delimiter', {sprintf('\t')})


is:

ans =
  cell
  {1×1 cell}


with Matlab R2017a (Windows®).

I don't think that the code I'm working with uses double quotes (cf. 'a' variable)...

Motius <motius>
Thu 23 Nov 2017 11:18:59 PM UTC, comment #7: 

Can the original reporter try the following in Matlab?


a = "\t\ta\tb\tc\n"
textscan(a, '%s', 'delimiter', {sprintf('\t')})


In Octave, this is returning a 4x1 cell array.  Whereas with just '\t' a 5x1 cell array is returned.

Also, I updated the patch from comment #2 and added BIST tests for this bug report in file-io.cc.





(file #42464)

Rik <rik5>
Group administrator
Thu 23 Nov 2017 10:17:54 PM UTC, comment #6: 

Aside from the larger architectural issues around strread and textscan, is the patch in comment #2 good enough to resolve this bug?  I can review for style and commit if it is.

As for tests, there are already a bunch of tests for textscan in the file file-io.cc.  Try


test file-io.cc


to run them.


Rik <rik5>
Group administrator
Thu 23 Nov 2017 08:53:43 PM UTC, comment #5: 

Indeed the plan has always been to morph textscan() into a backend for strread. In fact I started that soon after Lachlan's binary textscan was accepted into core Octave but I didn't pursue that due to lack of time, and didn't pick up again.

At the time I tried to make strread.m as Matlab-compatible as I could; I think unconditionally removing spaces isn't the right thing to do.


Note that current strread.m can do some things that Matlab's strread nor Octave&Matlab's textscan can do, i.e., processing cuddling literals; see e.g., this strread.m test:

## Allow cuddling %sliteral but warn that it is ambiguous
%!warning <Ambiguous '%s' specifier immediately before literal in column 1>
%! [a, b] = strread ("abcxyz51\nxyz83\n##xyz101", "%s xyz %d");
%! assert (a([1 3]), {"abc"; "##"});
%! assert (isempty (a{2}), true);
%! assert (b, int32([51; 83; 101]));


and I often use that capability so I'd like to keep strread.m somehow.

As to tests, I'm sure there have been tests for binary textscan() but I didn't see them anymore in the fntests.log

Philip Nienhuis <philipnienhuis>
Group Member
Wed 22 Nov 2017 10:54:29 PM UTC, comment #4: 

comment #3: OK, having read the textscan help, the takeaway is that textscan is a superset of strread() except for possibly different treatment of whitespace characters?  Hence, it is difficult to make one or the other a main source routine and re-using it for the other routine?  strread is a script, so is it slower in some significant way?  It might be better if strread() utilized textscan() similar to what you described with "whitespace set to "\t\b"" (instead of going the other way with " \t\b".

Here's the white-space treatment from strread.m:


    ## For numeric fields, whitespace is always a delimiter, but not for text
    ## fields
    if (isempty (regexp (format, '%\*?\d*s')))
      ## Add whitespace to delimiter set
      delimiter_str = unique ([white_spaces delimiter_str]);
    else
      ## Remove any delimiter chars from white_spaces list
      white_spaces = setdiff (white_spaces, delimiter_str);
    endif


Unlike the above, the patch I submitted is always removing the delimiters regardless of whether the format is %s or numeric.

As an aside, it would be nice if built-in textscan() had tests just like scripts have tests so that one could tweak some code and immediately get feedback on what the ramifications are.

Dan Sebald <sebald>
Wed 22 Nov 2017 10:31:16 PM UTC, comment #3: 

Swapping delimiters from whitespace was also done in the old .m-version of textscan (actually in its workhorse strread.m).

FWIW:
What also might need some attention (ISTR I mentioned this earlier as well) is that as soon as a "%s" (string) format conversion specifier occurs, whitespace behaves a bit differently.
That was my primary motive to add the '"whitespace", ""' option/value pair in comment #1.
This is where the default whitespace & delimiter characters collection comes in.

All this has been implemented in strread.m, see L.409-428. Note that textscan.m called strread.m with default (= if not overridden) whitespace set to (IIRC) " \t\b" (note space). These days I'm unsure if space really belongs to Matlab's textscan default whitespace collection.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 22 Nov 2017 08:12:00 PM UTC, comment #2: 

It looks like the scanning routines all do skipping of white space in some form:


  textscan::scan_one (delimited_stream& is, const textscan_format_elt& fmt,
                      octave_value& ov, Array<octave_idx_type> row)
  {
    skip_whitespace (is);


However, at the instantiation, the default white space includes the \t character:


  textscan::textscan (const std::string& who_arg)
    : who (who_arg), buf (), whitespace_table (), delim_table (),
      delims (), comment_style (), comment_len (0), comment_char (-2),
      buffer_size (0), date_locale (), inf_nan (init_inf_nan ()),
      empty_value (numeric_limits<double>::NaN ()), exp_chars ("edED"),
      header_lines (0), treat_as_empty (), treat_as_empty_len (0),
      whitespace (" \b\t"), eol1 ('\r'), eol2 ('\n'),
      return_on_error (1), collect_output (false),
      multiple_delims_as_one (false), default_exp (true), lines (0)
  { }


To confirm this hypothesis, note that using a comma as a delimiter works:


octave:1> a = ",,a,b,c\n"
a = ,,a,b,c

octave:3> textscan(a, '%s', 'delimiter', sprintf(','))
ans =
{
  [1,1] =
  {
    [1,1] =
    [2,1] =
    [3,1] = a
    [4,1] = b
    [5,1] = c
  }

}


What needs to be done is remove any delimiters from the whitespace string. That is, if the user selects


'delimiter', sprintf('\t'))


as you've done, then that character should be subtracted from the string.

I'm attaching a patch that does just that.  It seems to be in a reasonable location for modifying the whitespace list, although I do see a lot of "isspace()" used throughout the textscan code; so whether the patch catches everything, I'm not sure.  Perhaps JWE and Rik can review for a more efficient use of strings or general fix.

(file #42460)

Dan Sebald <sebald>
Wed 22 Nov 2017 06:52:40 PM UTC, comment #1: 

Confirmed on dev octave.

The following may be a clue:

>> textscan(a, '%s', 'delimiter', "\t", "whitespace", "")
ans =
{
  [1,1] =
  {
    [1,1] =
    [2,1] =
    [3,1] = a
    [4,1] = b
    [5,1] = c
  }

}


...so this may have to do with the default whitespace / delimiter collection.

ISTR to have mentioned this (= default whitespace/delimiter) earlier on in another bug report about textscan in Octave >= 4.2.x

Philip Nienhuis <philipnienhuis>
Group Member
Wed 22 Nov 2017 01:11:33 PM UTC, original submission:  

SYNOPSIS

The following snippet illustrates a regressions in the textscan GNU octave function.

EXAMPLE



a = "\t\ta\tb\tc\n"
textscan(a, '%s', 'delimiter', sprintf('\t'))


RESULTS


Gives the following result under GNU octave v4.0.3:

{
  [1,1] =
  {
    [1,1] =
    [2,1] =
    [3,1] = a
    [4,1] = b
    [5,1] = c
  }
}


and the following result under GNU octave v4.2.1 (1a8b7b3ae5f2):

{
  [1,1] =
  {
    [1,1] = a
    [2,1] = b
    [3,1] = c
  }
}


MATLAB


The second one differs from MatLab behaviors.

OTHER


History


I initially reported this bug in the comments of bug 50485:
https://savannah.gnu.org/bugs/?50485#comment3

Tests


This was tested on debian GNU/Linux 9.1:
- Octave 4.0.3 in the repositories
- Octave 4.2.1 compiled from source (hg id: 1a8b7b3ae5f2)

Motius <motius>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #42464:  52479.cset1 added by rik5 (3KiB - 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 sebald (Updated the item)
  • -email is unavailable- added by motius (Submitted the item)
  • -email is unavailable- added by motius
  •  

    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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-29 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2017-11-25 rik5 Item GroupRegression Matlab Compatibility
        StatusNeed Info In Progress
        Release4.2.1 dev
    2017-11-24 rik5 StatusNone Need Info
    2017-11-23 rik5 Attached File- Added 52479.cset1, #42464
    2017-11-22 sebald Attached File- Added octave-textscan_whitespace_delimiter-djs2017nov22.patch, #42460
    2017-11-22 motius Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code