bugGNU Octave - Bugs: bug #54661, textscan() continues from next...

 
 

bug #54661: textscan() continues from next line if line ends with delimiter

Submitter:  None
Submitted:  Thu 13 Sep 2018 09:49:06 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Confirmed Assigned to:  None
Originator Name:  aj Originator Email:  -email is unavailable-
Open/Closed:  * Open Release:  * dev
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 03 Jun 2019 07:47:44 PM UTC, comment #11: 

What is the status of this bug?
It is still there in 6.0.0 (30f53f7a7293)

The two commands below produce the same output they shouldn't.

> c = textscan("0.0;w;x\n2.0;;b\n", "%f;%s;%s",'Delimiter',';', 'MultipleDelimsAsOne', false)
> c = textscan("0.0;w;x\n2.0;b;\n", "%f;%s;%s",'Delimiter',';', 'MultipleDelimsAsOne', false)


The output I get for both commands is

c =
{
  [1,1] =

     0
     2

  [1,2] =
  {
    [1,1] = w
    [2,1] = b
  }

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

}


Juan Pablo Carbajal <juanpi>
Group Member
Mon 17 Sep 2018 09:22:38 PM UTC, comment #10: 

Yes, strread() had the bug fixed with bug #44750.  So that is why I wondered about "bug #44750?" appearing in the source code.  In all the changeset I've attached and so on, I've immediately deleted the comment from


    if (multiple_delims_as_one)           // bug #44750?
      skip_delim (is);


It seems like something someone took a guess at after the fact.

Dan Sebald <sebald>
Mon 17 Sep 2018 07:54:39 PM UTC, comment #9: 

I just checked strread.m and it doesn't have this bug; it just needs the EOL character specified through the (undocumented in strread.m) "endofline" parameter.
textread.m sets it for strread.m (which happens to be textread's work horse) and then strread.m does it right:

>> [a, b, c, d] = textread ("bug_example.txt", "%f %f %f %f", "delimiter", ",")
debug> dbcont
a =
    1
    5
    9
   13

b =
    2
    6
   10
   14

c =
     3
     7
   NaN
    15

d =
     4
     8
   NaN
    16


Philip Nienhuis <philipnienhuis>
Group Member
Sun 16 Sep 2018 10:12:51 PM UTC, comment #8: 

Yeah, there could be something going on with the buffer action.  The code has a lot of "attempt to process then put back to previous state if failed" kind of thing.  For example, automatically increment the line number, but then if EOLstop is specified decrement it back to where it was.  I might have prefered if a string buffer were used to store the ASCII hunk between two delimiters, then process, i.e., ask, Is this segment empty?  Can a floating point be extracted?  Is there character garbage left after the number conversion?  Etc.

I've made some adjustments which seem for the better, but then it always seems to fail one of the BIST tests.  It's as if there are some contradicting test cases.  For example, this


%! str = "12.234e+2,34, \n12345.789-9876j,78\n,10|3";
%! c = textscan (str, "%10.2f %f", "delimiter", ",", "collectOutput", 1,
%!                    "expChars", "e|");
%! assert (c, {[1223, 34; 12345.79-9876j, 78; NaN, 10000]}, 1e-6);


treats the \n as a whitespace, but I think that comes about simply because of the "skip whitespace" routine/scan.  Whereas if one were to treat \n as a delimiter the third field would be empty.

Matlab documentation doesn't give any examples of poorly-formatted lines.  It does specify though that '\n' and '\r' may be delimiters.  (And that by default '\n', '\r' and "\n\r" are EOL symbols.)  If one is allowed to specify those characters as delimiters, I'd think that by default they are not delimiters and shouldn't be in the list.

I'll keep looking at this a bit.  It seems to me from a programming standpoint that the code isn't keeping track of the presence of delimiters.  That would be an important piece of information as to whether a field is empty or has failed for another reason.  For example, the OP's example clearly should have empty fields that get transformed to "EmptyValue".  If the example were missing a comma and have too few delimiters, then that would be an error.

Dan Sebald <sebald>
Sun 16 Sep 2018 09:25:44 PM UTC, comment #7: 

BTW, I saw that the value '16' (the very last field in the file) apparently returns "out of phase" in the output.
Maybe there is something going on with putting back characters in the read buffer?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 16 Sep 2018 09:19:27 PM UTC, comment #6: 

@Dan:
yes I've been working at textscan.m & strread.m but that was years ago when textscan.m was a mere .m-file wrapper for strread.m that just added some bells & whistles. Now that textscan is a C++ function and ripped apart and scattered over various other internal functions, and because my C/C++ proficiency is a bit lacking, I've let textscan() go.
I think the main part of my contribution that still stands is some overview of the way delimiters, whitespaces and endoflines interfere with each other. (And even that is up for debate these days now that Matlab's docs get better and less ambiguous.)

EOLs are implicitly part of the delimiter collection, unless declared whitespace by the user. Or something like that :-)
AFAIU for textscan EOLs are not that special, unlike e.g., dlmread, so that one data row can extend across several lines, again unlike dlmread. That EOLs are not special is evident because if in the OP's example the EOLs are replaced by any another delimiter there is no more bug.

I have little time the next weeks (September is always a very busy month at work); but I'll try to find out why strread.m has the same bug. I think it has some tests for this case, but apparently not stringent enough.

From what I see I conclude that the last empty field is simply not recognized properly because of the way EOLs are interpreted/treated by both textscan() and strread.m; but you already came to that conclusion :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 16 Sep 2018 04:19:07 AM UTC, comment #5: 

It seems to me that there is a flaw in the way the delimiters is constructed.  The newline characters (eol1 and eol2) are included in the list of delimiters:


    bool is_delim (unsigned char ch) const
    {
      return ((delim_table.empty () && (isspace (ch) || ch == eol1 || ch == eol2))
              || delim_table[ch] != '\0');
    }


and


    // Create look-up table of delimiters, based on 'delimiter'
    delim_table = std::string (256, '\0');
    if (eol1 >= 0 && eol1 < 256)
      delim_table[eol1] = '1';        // EOL is always a delimiter
    if (eol2 >= 0 && eol2 < 256)
      delim_table[eol2] = '1';        // EOL is always a delimiter


There's fine control lost as a result because sometimes the newline character is supposed to behave differently.  In the following hunk of code, see how the character c1 is checked for being a delimiter, i.e., is_delim (c1):


  // Skip delimiters -- multiple if MultipleDelimsAsOne specified.
  int
  textscan::skip_delim (delimited_stream& is)
  {
    int c1 = skip_whitespace (is, true);  // 'true': stop once EOL is read
    if (delim_list.numel () == 0)         // single character delimiter
      {
        if (is_delim (c1) || c1 == eol1 || c1 == eol2)
          {
            is.get ();
            if (c1 == eol1 && is.peek_undelim () == eol2)
              is.get_undelim ();          // if \r\n, skip the \n too.


Checking for c1 == eol1 or eol2 is superfluous as is_delim() will test true based on the fact that eol1 and eol2 are currently in the list of delimiters.  The thing is, there are circumstances where the EOL should not be discarded.  I went about adding a EOLstop to the skip_delim() just as there is with skip_whitespace(), i.e.,


        if (is_delim (c1) || ((c1 == eol1 || c1 == eol2) && ! EOLstop))
          {
            is.get ();


but that doesn't do anything in light of what I just pointed out.

Any agreement on the fact that the EOL characters can't be included within the list of delimiters?  I think those need to be tested separately based on the whether the EOL should be skipped or not.  Maybe we could conditionally include the EOL characters in the list of delimiters, but because of the two-character EOL sequence \r\n I think it might be best to keep them separate.

Dan Sebald <sebald>
Sun 16 Sep 2018 12:57:52 AM UTC, comment #4: 

Attached is a patch which is not 100% complete.  To make things correct would require much more modification and testing than I can do at the moment.

With regard to this particular bug, the fundamental issue is that the scanning process for an individual value skips EOL.  Hence, something like 9,10,, is processed as


scan the 9 -->

,10,,

skip delim -->

10,,

scan the 10 -->

,,

skip delim -->

,

scan the NAN -->

,

skip delim -->



scan the ??? 13 -->

,14,15,16


So, I believe I have addressed that with the patch.

However, the changeset results in there being a failed test, i.e.,


octave:7> test <path>/libinterp/corefcn/file-io.cc
***** test
 str = "12.234e+2,34, \n12345.789-9876j,78\n,10|3";
 c = textscan (str, "%10.2f %f", "delimiter", ",", "collectOutput", 1,
                    "expChars", "e|");
 assert (c, {[1223, 34; 12345.79-9876j, 78; NaN, 10000]}, 1e-6);
!!!!! test failed
ASSERT errors for:  assert (cond {i},expected {i},tol)

  Location  |  Observed  |  Expected  |  Reason
     .          O(4x2)       E(3x2)      Dimensions don't match


One can see what the issue is in this case.  The extra delimiter ", \n" doesn't agree with the format string, i.e., 2 fields.  But when the patch removes the code that skips the delimiter, white space and EOL, the patched code ends up scanning an extra value for the ", \n".  This would be an easy fix: just skip to after the next EOL character after calling textscan::read_format_once(), which reads one line in the file according to the format specified.

The problem is that the current code conflates the delimiter skip and EOL skip in the same textscan::skip_delim() run for every individual field.  That simply can't be done.  Just think about it for a while: the fields all have to be scanned, then we consider skipping past the EOL.  If there is an EOL skip within the individual fields (i.e., the field loop) there's no way after that fact to determine whether an EOL has been skipped or not... so if we automatically skip EOL after having read individual fields we could end up going past the next line.

So,


  textscan::skip_delim (delimited_stream& is)
  {


needs to be redone, but there is too much eol1 equal this, eol2 equal that, etc.

(file #45019)

Dan Sebald <sebald>
Sat 15 Sep 2018 07:49:01 PM UTC, comment #3: 

@Philip: It appears you worked on this a bit in the past, so I'll toss some observations your way.  Looking through the code, the following caught my attention:

libinterp/corefcn/oct-stream.cc


    if (multiple_delims_as_one)           // bug #44750?
      skip_delim (is);


But the question mark bug report

https://savannah.gnu.org/bugs/?44750

is closed as fixed.  I don't see the above comment added as part of the bug report either, so how it got in there, I don't know.  Not that important (there should be an easy way of searching for this in thg but I can't find one), but the point is that the bug solved in strread.m

http://hg.savannah.gnu.org/hgweb/octave/rev/f68c3a62e42c

seems to be very similar.  Perhaps it's the same issue here.  But clearly it doesn't have to do with that multiple_delims_as_one parameter because that feature seems to work correctly.

I've printed out the c1 character coming back from skip_whitespace() for O.P.'s example:


octave:2> values = textscan(f_in, '%f %f %f %f', 'Delimiter',',');
skip_whitespace: 44  [1] (my manual addition)
skip_whitespace: 44  [2]
skip_whitespace: 44  [3]
skip_whitespace: 10  [4]
skip_whitespace: 44  [5]
skip_whitespace: 44  [6]
skip_whitespace: 44  [7]
skip_whitespace: 10  [8]
skip_whitespace: 44  [9]
skip_whitespace: 44  [10]
skip_whitespace: 44  [11]
                            (Why isn't a 10 or some other
                             character appearing here?)
skip_whitespace: 44  [12]
skip_whitespace: 44  [13]
skip_whitespace: 44  [14]
skip_whitespace: 10  [15]
skip_whitespace: -1


It looks like this code isn't being run for that empty field at end of line


  // Skip delimiters -- multiple if MultipleDelimsAsOne specified.
  int
  textscan::skip_delim (delimited_stream& is)
  {
    int c1 = skip_whitespace (is, true);  // 'true': stop once EOL is read
std::cerr << "skip_whitespace: " << c1 << "\n";


Dan Sebald <sebald>
Fri 14 Sep 2018 09:17:46 PM UTC, comment #2: 

There is an "EndOfLine" option to textscan, so one would think that it should fill in NaN when there aren't enough columns.  That definitely seems like a bug to me.

As an alternative for you, rather than creating cells since your data file is rather uniform in its elements, you might want to try dlmread():


octave:17> data = dlmread('bug_example.txt', 'emptyvalue', NaN)
data =

     1     2     3     4
     5     6     7     8
     9    10   NaN   NaN
    13    14    15    16


which appears to work properly and gives you a nice matrix to work with if that is what you eventual want.  (As opposed to stitching together a bunch of cell columns.)

So there is the original bug reported.  Also, I'd like to add that the above dlmread() routine requires that 'emptyvalue' be all lower case.  That is, it doesn't accept 'EmptyValue':


octave:18> data = dlmread('bug_example.txt', 'EmptyValue', NaN)
error: dlmread: error parsing RANGE


Should we change that to accept EmptyValue and be consistent with routines like textscan?

Dan Sebald <sebald>
Fri 14 Sep 2018 08:55:18 PM UTC, comment #1: 

Confirmed.
I think this may well be a duplicate of bug #52867, or is related to it. That said, the old strread.m has this same bug here but worked for bug #52867.

It is the EOL that somehow confuses textscan.
A way to read your data is to change the EOLs into a delimiter first:

>> f_in = fopen('bug_example.txt');
>> txt  = fread (f_in, Inf, "*char")';
>> fclose(f_in);
>> txt2 = strrep (txt, "\n", ",");
>> values = textscan(txt2, '%f %f %f %f', 'Delimiter', ',')
values =
{
  [1,1] =
      1
      5
      9
     13
  [1,2] =
      2
      6
     10
     14
  [1,3] =
       3
       7
     NaN
      15
  [1,4] =
       4
       8
     NaN
      16
}


Philip Nienhuis <philipnienhuis>
Group Member
Thu 13 Sep 2018 09:49:06 PM UTC, original submission:  

Examples are attached - a data file and a script which reads data from that file using textscan().

When textscan() reads the third line of the example file, it jumps to the fourth line and basically reads "9,10,NaN,13" instead of interpreting the third line as "9,10,NaN,NaN"

If you use textscan to the string '9,10,,' it outputs 9, 10, NaN, NaN which is logical.


I'm not sure if this is a bug or an intended feature, but it's confusing and very annoying anyway.

I can't check if Matlab does it this way or not. If it does, then this behavior shouldn't probably be changed. However, it would be useful if you could force textscan() to process exactly one line at a time.

Of course you can loop, manually read a line, use textscan(), and merge the results, but it's much less elegant.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45005:  bug_example.txt added by None (35B - text/plain)
file #45006:  bug_example.m added by None (181B - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by juanpi (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2018-09-16 sebald Attached File- Added octave-textscan_no_eol_skip-djs2018sep15.patch, #45019
    2018-09-14 philipnienhuis Item GroupNone Incorrect Result
        StatusNone Confirmed
        Release4.4.1 dev
    2018-09-13 None Attached File- Added bug_example.txt, #45005
        Attached File- Added bug_example.m, #45006

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code