bugGNU Octave - Bugs: bug #34734, problems with latest strread...

 
 

bug #34734: problems with latest strread (newlines, spaces and commas)

Submitter:  Kris Thielemans <krthie>
Submitted:  Thu 03 Nov 2011 12:25:29 PM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  philipnienhuis
Originator Name:  Kris Thielemans 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

Fri 23 Dec 2011 12:55:32 PM UTC, comment #17: 

With stable branch:

octave.exe:1> test strread
PASSES 23 out of 23 tests
octave.exe:2> test textread
PASSES 5 out of 5 tests
octave.exe:3> test textscan
PASSES 13 out of 13 tests
octave.exe:4>

(textscan & textread depend on strread)

so I'll close this bug.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 23 Dec 2011 12:05:52 AM UTC, comment #16: 

You scared me a bit ;-)


test strread
PASSES 23 out of 23 tests


You need to use the "stable" branch


hg pull
hg update -C stable
make


Ben Abbott <bpabbott>
Group Member
Thu 22 Dec 2011 10:58:51 PM UTC, comment #15: 

I've pulled and built Octave (on MinGW).
strread.m is still the previous version (the latest patches are missing):

Philip@deskprn ~/octave/octave-3.5.0+
$ ./run-octave
GNU Octave, version 3.5.90+
:
<snip>
:
octave.exe:1> test strread
PASSES 17 out of 17 tests
octave.exe:2> test textread
PASSES 5 out of 5 tests
octave.exe:3> test textscan
PASSES 13 out of 13 tests
octave.exe:4> which strread
`strread' is a function from the file X:\programs\msys\home\Philip\octave\octave-3.5.0+\scripts\io\s
trread.m

The strread.m I sent you has 23 tests.....

Or did I check out the wrong branch? I did:
$ hg clone http://www.octave.org/hg/octave

Philip Nienhuis <philipnienhuis>
Group Member
Thu 22 Dec 2011 05:05:52 PM UTC, comment #14: 

Done.

http://hg.savannah.gnu.org/hgweb/octave/rev/153581342e47

Since we are close to a release, please pull and test drive it to be sure everything works.

Ben Abbott <bpabbott>
Group Member
Thu 22 Dec 2011 04:40:05 PM UTC, comment #13: 

Thx Ben, will you push it?

Philip Nienhuis <philipnienhuis>
Group Member
Wed 21 Dec 2011 10:19:05 PM UTC, comment #12: 

I've converted Phil's strread.m to a changeset.

All related tests pass for me.

(file #24646)

Ben Abbott <bpabbott>
Group Member
Wed 21 Dec 2011 07:54:55 PM UTC, comment #11: 

Thanks Ben & Jordi.

I'll see what I can do. Hopefully in time for 3.6.0

(Fixing strread & friends seems an endless affair :-( )

Philip Nienhuis <philipnienhuis>
Group Member
Wed 21 Dec 2011 05:00:59 PM UTC, comment #10: 

Thanks Jordi. With the rebased changeset;


test strread
PASSES 19 out of 19 tests
test textscan
PASSES 13 out of 13 tests
test text
text      textread  textscan
test textread
  ***** test
 f = tmpnam();
 d = rand (5, 3);
 dlmwrite (f, d, 'precision', '%5.2f');
 [a, b, c] = textread (f, "%f %f %f", "delimiter", ",", "headerlines", 3);
 unlink(f);
 assert (a, d(4:5, 1), 1e-2);
 assert (b, d(4:5, 2), 1e-2);
 assert (c, d(4:5, 3), 1e-2);
!!!!! test failed
assert (a,d (4:5, 1),1e-2) expected
   0.96669
   0.41522
but got
   0.97000
       NaN
       NaN
   0.45000
Dimensions don't match


After reverting the strread.m changeset


test textread
PASSES 5 out of 5 tests


I ran fntests() and didn't see any other problems.

The test failure for textread() is due to a new bug in strread(). The script below illustrates the problem.


str =  "0.31, 0.86, 0.94\n 0.60, 0.72, 0.87";
fmt = "%f %f %f";
args = {"delimiter", ",", "endofline", "\n", "whitespace", " "};
[a, b, c] = strread (str, fmt, args {:});
assert (a, [0.31; 0.60], 0.01)
assert (b, [0.86; 0.72], 0.01)
assert (c, [0.94; 0.87], 0.01)


The first assert throws an error.


error: assert (a,[0.31; 0.60],0.01) expected
   0.31000
   0.60000
but got
   0.31000
       NaN
       NaN
   0.87000
Dimensions don't match


With the commas (",") removed from str, the script runs without error.

With all the spaces removed form str, the script also runs without error.

I've attached a modified changeset with additional tests added. The last test fails per above.

(file #24641)

Ben Abbott <bpabbott>
Group Member
Wed 21 Dec 2011 03:32:46 PM UTC, comment #9: 

I did


hg import --exact strread.2.patch


to apply the patch at the location in which Philip wrote it. However, Mercurial complains that the patch is corrupt. After some investigation, it appears that the problem is that the last hunk should be at


@@ -810,9 +836,14 @@


instead of


@@ -810,9 +836,11 @@


(notice the last number is 14, not 11).

I corrected this and rebased the patch to the stable branch. I attach it.

(file #24640)

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Wed 21 Dec 2011 02:27:25 PM UTC, comment #8: 

Phil,

I tried to apply "strread.2.patch" to the stable branch. Unfortunately, it doesn't apply cleanly for me. Can you generates this again for the stable branch ?

Ben

Ben Abbott <bpabbott>
Group Member
Mon 28 Nov 2011 09:54:21 PM UTC, comment #7: 

Please try attached new patch.

The error you saw is fixed, try-catch is restored, a fix added for yet another problem I hit (eol_char in subfunc split_by), and I added %!test lines to the last two tests you added so now they're counted too.

I think strread's functionality is stretched to the limit now.

I didn't add tests as you requested, partly because the new functionality is included in the other tests, mostly because str2double() has to be patched first (see bug #34713) before I can proceed.

(file #24491)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 27 Nov 2011 11:20:46 PM UTC, comment #6: 

I just noticed that the chageset below commented out a try/catch block.

http://hg.savannah.gnu.org/hgweb/octave/diff/f5a3f77d51aa/scripts/io/strread.m

Was that intentional?


Ben Abbott <bpabbott>
Group Member
Sun 27 Nov 2011 07:00:32 PM UTC, comment #5: 

I applied the strread.patch to the development sources and when I ran the strread tests I encountered the error below.


test strread
ans =

   8   9  10  13  32

  ***** test
 [a, b] = strread ("1 2", "%f%f");
 assert (a, 1);
 assert (b, 2);
!!!!! test failed
`mxd_fmt' undefined near line 417 column 24


Also, once that is fixed, can a test be added for the new functionality?

Ben Abbott <bpabbott>
Group Member
Sun 06 Nov 2011 10:02:36 PM UTC, comment #4: 

After having looked at this in more detail, I'm afraid I'll have to give up on this one.

This is due to the way strread has been made by Eric Chassande and later Jaroslav & Soren:
the input text string is split up into a cell array along delimiters; later the data is parsed, assuming that the "data columns" correspond to cells at regular intervals in the cell array, where the interval (periodicity) equals the number of data columns.

Now, ML seems to have two (or more?) levels of delimiters:
(1) Whitespace (always for numeric fields) - but see (3) below;
(2) Delimiters (which for text fields replace whitespace, but for numeric fields seem to augment whitespace)
(3) Moreover it turns out that ML doesn't actually uses whitespace or delimiters for numeric fields, it rather just quits interpreting a numeric field if the next character doesn't fit into a number template; it then assumes the next character is part of the next field (e.g., look at C(2)):

>> C = textscan ('1a 2 3 , 4 5, , 6', '%d%s', 'delimiter', ',', 'whitespace', '');
>> a = C(1); a{:}

ans =
           1
           4
           0

>> b = C(2); b{:}

ans =
    'a 2 3 '
    ' 5'
    ' 6'
   
(ML's strread behaves identically)

ML's interpretation scheme simply cannot be implemented robustly in the Octave implementation. I see no way that two types of "field delimiters" can be invoked, especially if they do not line up vertically; Octave's scheme will break then because mixed data type columns may get "out-of-phase. The above example is a nice illustration.

ML's way of interpeting data can only be mimicked by a routine that linearly ploughs through a text file and, at each end-of-field, determines what to do based on the individual format conversion specifier in turn.

I've prepared a changeset based on the rough patch I gave earlier, augmented by Ben's patches for the texinfo header + some overdue patches I submitted a while ago in bug #33971.
As to current strread, I'm afraid that is far as we can stretch its performance.

Hopefully jwe's upcoming compiled textscan will be more powerful.

Ben will you please review this changeset? TIA

(file #24315)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 03 Nov 2011 08:24:33 PM UTC, comment #3: 

Yep, I didn't correctly "parse" your last example, sorry. "\n" referred to the one but last example.

Your inference of how str2double() mixes things up is correct.

To clarify, the bug to fix is that in ML the whitespace set always seems to be a delimiter for numeric fields, and the delimiter parameter set only works for string (text) fields; however Octave only accepts the delimiter set as delimiters, for any field type.
So I have to add some magic to split off numeric fields columns separated by whitespace.

I attached a very raw & kludgy patch to the dev sources that I think will work as long as you have no mix of string (%s) and numeric conversion modifiers in your format string. This patch is easy enough to apply by hand.
Pls report if this works for you.

With this patch, strread.m still passes all current tests here (though I may need to revise them all in light of Ben's new set of tests for textscan).


(file #24297)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 03 Nov 2011 06:25:06 PM UTC, comment #2: 

Hi

wow, I hadn't seen that thread on the maintainer list. You and Ben certainly persist in digging into this. I should probably leave this to you therefore.

However, I cannot resist a comment on your comments (sorry!). You say

In your final example, "\n" is no "record delimiter" as strread reads from a string, not a text file.

This is strange as my final example in the post doesn't have any newlines. But I now think I understand that the last example

octave> [a1,a2]=newstrread("1     2, 3 4",'%f%f','delimiter','t')

is also a consequence of the strdouble bug. I'm guessing that strread correctly passes the string "1   2,3 4" as the first field to strdouble (as there are no delimeters in sight), which then makes a mess of it.

So, upshot of all this, I think you correctly zeroed-in on the bug to fix (i.e the \n as delimiter) for strread, and the str2double bug is separate. Note however that the same "comma" behaviour happens for %d:

octave:5> [a1,a2]=strread('1,2,3 5 6','%d%d')
a1 =

  123
    6

a2 = 5

but maybe you're using str2double before going to int32.

thanks for all the effort!



Kris Thielemans <krthie>
Thu 03 Nov 2011 05:21:54 PM UTC, comment #1: 

(Devs: are bugs in script files "libraries" or "interpreter" bugs?)

Several more comparitive tests w ML & Octave led to the conclusion that ML documentation is not very clear about the exact behavior of strread & textscan. In some instances ML even deviates from its documented behavior.
There's still no agreement as to what ML behavior should be considered a bug, or just a case of poor documentation. See this thread:
https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2011-October/025443.html

I know how to fix your case 1 (it'll incur another speed penalty BTW), but I doubt if every ML corner case behavior can be mimicked; this is largely due to the way Octave's strread.m has been written.

In your second case you haven't specified a comma as delimiter. The bug here is actually due to str2double()  (Happens in your last case as well)

In your final example, "\n" is no "record delimiter" as strread reads from a string, not a text file.
In fact, "\n" (like " ", "\t", "\b" and "\r") should rather have been included in the delimiter set for numeric fields only by default. That (rather obscurely documented ML behavior) is the exact bug I'm going to (try to) fix.

Note that 3.2.4 strread has the "inverse bug" when reading a string using "%s" format conversion specifiers. In that case space should NOT be part of the delimiter set for strings.

Thx for reporting.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 03 Nov 2011 12:25:29 PM UTC, original submission:  

This was originally discussed on the Octave- general mailing list (See the thread http://octave.1599824.n4.nabble.com/problems-with-latest-strread-comma-delimiters-tt3913839.html#a3919252). Some behaviour reported here is related to bug #34713 (https://savannah.gnu.org/bugs/index.php?34713)


I'm trying to use textread but had trouble on Octave 3.4.2 (built from source on Ubuntu 11.04). Therefore, I downloaded the latest strread.m from mercurial, renaming it to newstrread.m.

However, newstrread behaves unexpectly with commas/spaces and newlines.


    octave>  [a1,a2]=newstrread("1,2\n3,4\n", '%f%f', 'delimiter',',')
    a1 =
       1
       4
    a2 =
        23
       NaN


The newline was not considered as a "new record" and just ignored. Instead, the 3.2.4 version of strread reads this correctly:

    octave>  [a1,a2]=strread("1,2\n3,4\n", '%f%f', 'delimiter',',')
    a1 =
       1
       3
    a2 =
       2
       4


Here's another case where both the 3.2.4 and new strread (and textread) ignore some unknown characters, which causes rather strange results:

    octave>  [a1,a2]=newstrread("1a2\n3,4", '%f%f')
    a1 = NaN
    a2 =  34

Returning a NaN for the first line is ok, but ignoring the comma is wrong (see bug #34713).

In the discussion in the email thread, it appeared that this behaviour is partly due to the fact that currently strread doesn't interpret a new line as a new "record", which seems incorrect.

Another example without newlines

octave> [a1,a2]=newstrread("1     2, 3 4",'%f%f','delimiter','t')
a1 =  1234
a2 = [](0x1)

IMO, whitespace and commas should not be ignored "inside" a field (whitespace should of course be ignored between fields when using the above format specifier).




Kris Thielemans <krthie>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #24646:  changeset.patch added by bpabbott (12KiB - application/octet-stream - Phil's strread.m converted to a changeset)
file #24641:  strread-rebased-with-tests.patch added by bpabbott (12KiB - application/octet-stream)
file #24640:  strread-rebased.patch added by jordigh (11KiB - text/x-diff)
file #24491:  strread.2.patch added by philipnienhuis (11KiB - application/octet-stream)
file #24315:  strread.patch added by philipnienhuis (7KiB - 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 jordigh (Updated the item)
  • -email is unavailable- added by bpabbott (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by philipnienhuis (Posted a comment)
  • -email is unavailable- added by krthie (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-12-23 philipnienhuis StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2011-12-22 bpabbott StatusPatch Submitted Ready For Test
    2011-12-21 bpabbott Attached File- Added changeset.patch, #24646
    2011-12-21 bpabbott Attached File- Added strread-rebased-with-tests.patch, #24641
    2011-12-21 jordigh Attached File- Added strread-rebased.patch, #24640
    2011-11-28 philipnienhuis Attached File- Added strread.2.patch, #24491
    2011-11-06 philipnienhuis Attached File- Added strread.patch, #24315
        StatusIn Progress Patch Submitted
        Carbon-Copy- Added bpabbott
    2011-11-03 philipnienhuis Attached File- Added provsnl_strread.m.diff, #24297
    2011-11-03 philipnienhuis StatusNone In Progress
        Assigned toNone philipnienhuis

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code