bugGNU Octave - Bugs: bug #52550, textscan drops delimiter character...

 
 

bug #52550: textscan drops delimiter character for multi-character, cell-specified delimiter option

Submitter:  Dan Sebald <sebald>
Submitted:  Wed 29 Nov 2017 05:02:37 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

Sat 02 Dec 2017 05:27:53 AM UTC, comment #13: 

Pushed the last cset here http://hg.savannah.gnu.org/hgweb/octave/rev/f77da8da0f3f.


Marking as fixed and closing report.

Rik <rik5>
Group administrator
Fri 01 Dec 2017 11:35:15 PM UTC, comment #12: 

OK, thanks.  I update again with a couple extra hunks to do the following:

(textscan::read_until): Treat eol1 and eol2 as delimiters.
(textscan::scan_string): Add eol1 and eol2 characters to "ends" string array.

It's odd that in the following:


octave:13> str = "99end2 space88gap 4564";
octave:14> c = textscan (str, "%d %s", "delimiter", {"end", "gap", "space"})
c =
{
  [1,1] =

    99
    88

  [1,2] =
  {
    [1,1] = 2
    [2,1] = 4564
  }

}

octave:15> str2 = "99,2 ,88, 4564";
octave:16> c2 = textscan (str2, "%d %s", "delimiter", ',')
c2 =
{
  [1,1] =

    99
    88

  [1,2] =
  {
    [1,1] = 2
    [2,1] = 4564
  }

}

octave:17> assert (c,c2)


that the space after the 2 is kept, but the space before the 4 is dropped.

(file #42549)

Dan Sebald <sebald>
Fri 01 Dec 2017 08:19:10 PM UTC, comment #11: 

I amended the patch to include the proposed tests in comment #10 as BIST tests in file-io.cc.  See the attached 52550.v2.cset.

One problem is that this BIST test is now failing


 %!test <52479>
 %! str = "\t\ta\tb\tc\n";
 %! ret = textscan (str, "%s", "delimiter", {"\t"});
 %! assert (ret, { {''; ''; 'a'; 'b'; 'c'} });


The issue is that the final string 'c' is being returned as "c\n".  This is not the case if the delimiter is just "\t" without being a cell array.


(file #42547)

Rik <rik5>
Group administrator
Fri 01 Dec 2017 12:08:15 AM UTC, comment #10: 

Attached is an updated patch with a fix for the numeric read when using multi-character (i.e., cell-specified) delimiters.  Here are some tests:


a = ",,1,2,3\n";
textscan(a, '%d', 'delimiter', ',')
textscan(a, '%d', 'delimiter', {','})
b = " , ,1,2,3\n";
textscan(b, '%d', 'delimiter', ',')
textscan(b, '%d', 'delimiter', {','})
c = " 0 , 5+6j , -INF+INFj ,NaN,3\n";
textscan(c, '%f', 'delimiter', ',')
textscan(c, '%f', 'delimiter', {','})
d = " 0;,;,1;,2;,3\n";
textscan(d, '%f', 'delimiter', {';,'})
e = " 0 ;1 , $ 2 ;3\n";
textscan(e, '%f', 'delimiter', ',;$')
textscan(e, '%f', 'delimiter', {',',';','$'})


The last example illustrates the equivalence between a character string and specifying all individual characters of that string within a cell.  In that sense, the C++ code doesn't really have to have two conditions throughout the code.  It could instead have just the multi-character delimiter code, and within the function parsing routine it translates the string ',;$' to a delim_list internally.  All depends how one wants to go forward with this routine and how it might fit into the textscan/load class of functions.

The goal here was to make the changes as minimal as possible and still be reasonably functional.  I'm not quite sure just how robust the overall code is with all the FIXMEs and the lookahead.  Seems to work in most cases I've looked at.

(file #42538)

Dan Sebald <sebald>
Wed 29 Nov 2017 07:49:01 PM UTC, comment #9: 

I'll look at it this evening again, so wait just a bit.  The example I gave of:


octave:35> b = " , ,1,2,3\n";
octave:36> textscan(b, '%d', 'delimiter', ',')
ans =
{
  [1,1] =

    0
    0
    1
    2
    3

}

octave:37> textscan(b, '%d', 'delimiter', {','})
ans =
{
  [1,1] = [](0x1)
}


is problematic.  It looks like it is the same type of bug still.  Inside of the routine textscan::read_double in oct-stream.cc is the following code which not only needs to put back a character if the value of i is zero, but notice how it is exactly repeated code:


    is.clear ();
    if (! used_exp && ch != std::istream::traits_type::eof () && width_left)
      is.putback (ch);

    // Check for +/- inf and NaN
    if (! valid && width_left >= 3)
      {
        int i = lookahead (is, inf_nan, 3, false);   // false -> case insensitive
        if (i == 0)
          {
            retval = numeric_limits<double>::Inf ();
            valid = true;
          }
        else if (i == 1)
          {
            retval = numeric_limits<double>::NaN ();
            valid = true;
          }
      }

    // Check for +/- inf and NaN
    if (! valid && width_left >= 3)
      {
        int i = lookahead (is, inf_nan, 3, false);   // false -> case insensitive
        if (i == 0)
          {
            retval = numeric_limits<double>::Inf ();
            valid = true;
          }
        else if (i == 1)
          {
            retval = numeric_limits<double>::NaN ();
            valid = true;
          }
      }


It looks to me as though someone intended for the second instance to be modified into checking for multi-character delimiters but never got around to it.  Just a guess, so this will take some time to figure out what the intention was and add a little loop for the second case similar to the textscan::read_until routine.

Dan Sebald <sebald>
Wed 29 Nov 2017 07:38:38 PM UTC, comment #8: 

Okay, my mistake.

Shall I go ahead and push your fix for the basic behavior?  I still don't know if we want this, but if it is present it should at least work.

Rik <rik5>
Group administrator
Wed 29 Nov 2017 07:27:20 PM UTC, comment #7: 

I agree, but I'm fine with sequestering the multi-char delimiter with a property.  Somebody put a lot of work into it.

That's a strange example you've given, but I think it's a matter of not being formatted correctly because the last command below makes sense:


octave:39> str = 'ABC1ABC2ABC3'
str = ABC1ABC2ABC3
octave:40> textscan (str, '%d %d %d', 'delimiter', {'ABC'})
ans =
{
  [1,1] =

    0
    3

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

octave:41> textscan (str, '%d %d %d %d', 'delimiter', {'ABC'})
ans =
{
  [1,1] = 0
  [1,2] = 1
  [1,3] = 2
  [1,4] = 3
}


I think in the middle example it is as though Octave is interpretting this as a collection of columns in an otherwise matrix.  That is, %d%d%d suggests to read the input as a matrix with three columns, so it comes out


   0   1   2
   3


Dan Sebald <sebald>
Wed 29 Nov 2017 06:54:44 PM UTC, comment #6: 

For comment #2, could we get a test of whether Matlab accepts multi-character delimiters?  If we don't need to support these in Octave then maybe we should just drop them and make our lives easier.

Here is a test case


str = ABC1ABC2ABC3
textscan ('%d %d %d', 'delimiter', {'ABC'})


Octave's answer is wrong, but it does accept multiple-character delimiters.


ans =
{
  [1,1] =

    0
    3

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


I'm pretty sure the answer really should be a cell array {0, 1, 2, 3}.  The empty value at the front of the string was recorded as 0, but somehow also snagged the final value of 3.

Rik <rik5>
Group administrator
Wed 29 Nov 2017 05:15:15 PM UTC, comment #5: 

As for compatibility, perhaps we could introduce some property (e.g., "MultipleCharacterDelimiter") for which if true causes the Delimiter (whether it be a string or cell of strings) to be treated as whole words as opposed to individual characters, i.e.,


textscan(a, '%d', 'delimiter', '123')
textscan(a, '%d', 'delimiter', {'123'})


would be treated as individual characters, whereas


textscan(a, '%d', 'delimiter', '123', 'MultipleCharacterDelimiter')
textscan(a, '%d', 'delimiter', {'123'}, 'MultipleCharacterDelimiter')


would be treated as whole words.

Or, it could be that the two can be combined something like:


textscan(a, '%d', 'delimiter', ',', 'MultipleCharacterDelimiter', '123')


Dan Sebald <sebald>
Wed 29 Nov 2017 05:01:34 PM UTC, comment #4: 

Something is still wrong/inconsistent here.  Note the difference between the two modes for integer input format (as opposed to string):


octave:32> a = ",,1,2,3\n";
octave:33> textscan(a, '%d', 'delimiter', ',')
ans =
{
  [1,1] =

    0
    0
    1
    2
    3

}

octave:34> textscan(a, '%d', 'delimiter', {','})
ans =
{
  [1,1] = [](0x1)
}

octave:35> b = " , ,1,2,3\n";
octave:36> textscan(b, '%d', 'delimiter', ',')
ans =
{
  [1,1] =

    0
    0
    1
    2
    3

}

octave:37> textscan(b, '%d', 'delimiter', {','})
ans =
{
  [1,1] = [](0x1)
}

octave:38>


Dan Sebald <sebald>
Wed 29 Nov 2017 06:19:05 AM UTC, comment #3: 

Attached is a small bug fix.  Not much explanation beyond what is in said in the changeset comment is needed.  Does this solve everything in terms of the multi-character (i.e., cell-specified) delimiter?  Not sure, but I don't want to think too much about that right now.  Try the patch for some scenarios I guess.  At least the following works:


octave:7> b = "987123654123456"
b = 987123654123456
octave:8> c = "987120654123456"
c = 987120654123456
octave:9> textscan(b, '%s', 'delimiter', {'123'})
ans =
{
  [1,1] =
  {
    [1,1] = 987
    [2,1] = 654
    [3,1] = 456
  }

}

octave:10> textscan(c, '%s', 'delimiter', {'123'})
ans =
{
  [1,1] =
  {
    [1,1] = 987120654
    [2,1] = 456
  }

}


(file #42515)

Dan Sebald <sebald>
Wed 29 Nov 2017 05:24:58 AM UTC, comment #2: 

Some observations:

textscan, generally, has an abundance of FIXME's throughout.

This bug is not a whitespace issue because the same behavior exists when the delimiter is 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

  }

}


Also, the dropping of the delimiter is in the recursive loop portion of the code, not prior to the loop because the number of empty entries for the latter case is always half that of the former:


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

}

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

  }

}


Dan Sebald <sebald>
Wed 29 Nov 2017 05:21:34 AM UTC, comment #1: 

I'm going to make some general (and perhaps rambling) comments in this post, and then in follow up posts address what should be a simple fix for the original problem.

----------

In the current code there is a comment about delim_table and delim_list:


    // Three cases for delim_table and delim_list
    // 1. delim_table empty, delim_list empty:  whitespace delimiters
    // 2. delim_table = look-up table of delim chars, delim_list empty.
    // 3. delim_table non-empty, delim_list = Cell array of delim strings


The above combinations ostesibly represent two conditions, i.e.:


    if (delim_list.numel () == 0)         // single character delimiter
      {
      }
    else                                  // multi-character delimiter
      {
      }


That is, in order to make this distinction of single/multi-character, the delim_list (or some other means of representing multiple conditions) needs to exist.  There are a number of questions I have regarding this.

First is whether this mulit-character delimiter is proper and/or useful.  Is this behavior supposed to be a superset of Matlab?  If the single character string and character strings within a cell were, instead, to behave in a similar way, we could just put the characters for all scenarios in the same delim_table ('delims' string at time of parsing) and drop the delim_list usage in the code.  Any user who chooses a multicharacter delimiter should be able to easily convert their data file to a single character by use of sed.

Second is if this multi-character delimiter is the desired behavior for Octave, then does the coding need to be as complex as the current code?  The processing for multi-character is sort of non-causal in the sense that it needs to look N-1 symbols ahead, where N is the length of the delimiter.  For example, say the delimiter one chooses is "123".  Then upon being presented with data that starts with 1, there are two scenarios.  In the next two examples


example: 987123654123456...  >>  987 654 456 ...
example: 987120654123456...  >>  987120645 456 ...
            ^


imagine being presented with the '1'.  It's not until seeing the '3' or the '0' that one knows how to treat the data.  As a result, the code has this "lookahead" state memory by which it grabs some data from the stream (say three characters) and then puts the file pointer back to the location prior to the reading of the data.  This is somewhat clumsy programming.  Yes, there are processing tasks that require more-sophisticated programming and the current code is neither overly complex nor uncommon non-causal techniques, but it is always a matter of complexity of the programming matching the complexity of the problem.  We just explored the speed of loading ASCII data (see http://savannah.gnu.org/bugs/?51871) in which was used "getline" like techniques to bring a whole line of data into memory.  If a similar thing were done here, i.e., first get a whole line of data then search that string for multiple-character delimiters, it would obviate the need for grabbing look-ahead data and tellg, seekg, etc.

Again, even a more elegant use of getline/parse for multiple-character delimiters should have one wondering if the necessitated complexity of such a requirement is appropriate for computer programming and data file format.  Is this multi-character delimiter feature something that ever gets used?


Dan Sebald <sebald>
Wed 29 Nov 2017 05:02:37 AM UTC, original submission:  

In a comment of a bug report related to lost whitespace delimiters in textscan

https://savannah.gnu.org/bugs/?52479#comment12

it was noted that for consecutive delimiters in an input string when a delimiter is 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

  }

}


Dan Sebald <sebald>

 

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

Attach Files:
   
   
Comment:
   

 

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 (Submitted the item)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-02 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2017-12-01 sebald Attached File- Added octave-cell_specified_delimiter-bug52550-djs2017dec01.patch, #42549
    2017-12-01 rik5 Attached File- Added 52550.v2.cset, #42547
        StatusConfirmed In Progress
    2017-12-01 sebald Attached File- Added octave-cell_specified_delimiter-bug52550-djs2017nov30.patch, #42538
    2017-11-29 rik5 StatusNone Confirmed
    2017-11-29 sebald Attached File- Added octave-cell_specified_delimiter-bug52550-djs2017nov28.patch, #42515

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code