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

 
 

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

Submitted by:  Kris Thielemans <krthie>
Submitted on:  Thu 03 Nov 2011 12:25:29 PM UTC  
 
Category: LibrariesSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: Philip Nienhuis <philipnienhuis>
Originator Name: Kris ThielemansOpen/Closed: Closed
Release: devOperating System: Any

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project MemberIn charge of this item.
Fri 23 Dec 2011 12:05:52 AM UTC, comment #16:

You scared me a bit ;-)

You need to use the "stable" branch

Ben Abbott <bpabbott>
Project 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>
Project MemberIn charge of this item.
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>
Project Member
Thu 22 Dec 2011 04:40:05 PM UTC, comment #13:

Thx Ben, will you push it?

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
Wed 21 Dec 2011 05:00:59 PM UTC, comment #10:

Thanks Jordi. With the rebased changeset;

After reverting the strread.m changeset

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.

The first assert throws an error.

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>
Project Member
Wed 21 Dec 2011 03:32:46 PM UTC, comment #9:

I did

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

instead of

(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>
Project Administrator
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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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.

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

Ben Abbott <bpabbott>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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
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:
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>
Project MemberIn charge of this item.
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.

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

Here's another case where both the 3.2.4 and new strread (and textread) ignore some unknown characters, which causes rather strange results:
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
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 File(s):
   
   
Comment:
   

Attached Files
file #24646:  changeset.patch added by bpabbott (12kB - application/octet-stream - Phil's strread.m converted to a changeset)
file #24641:  strread-rebased-with-tests.patch added by bpabbott (12kB - application/octet-stream)
file #24640:  strread-rebased.patch added by jordigh (11kB - text/x-diff)
file #24491:  strread.2.patch added by philipnienhuis (11kB - application/octet-stream)
file #24315:  strread.patch added by philipnienhuis (7kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jordigh (Updated the item)
  • -unavailable- added by bpabbott (Posted a comment)
  • -unavailable- added by philipnienhuis
  • -unavailable- added by philipnienhuis (Posted a comment)
  • -unavailable- added by krthie (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 23 Dec 2011 12:55:32 PM UTCphilipnienhuisStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Thu 22 Dec 2011 05:05:52 PM UTCbpabbottStatusPatch Submitted=>Ready For Test
    Wed 21 Dec 2011 10:19:05 PM UTCbpabbottAttached File-=>Added changeset.patch, #24646
    Wed 21 Dec 2011 05:00:59 PM UTCbpabbottAttached File-=>Added strread-rebased-with-tests.patch, #24641
    Wed 21 Dec 2011 03:32:46 PM UTCjordighAttached File-=>Added strread-rebased.patch, #24640
    Mon 28 Nov 2011 09:54:21 PM UTCphilipnienhuisAttached File-=>Added strread.2.patch, #24491
    Sun 06 Nov 2011 10:02:36 PM UTCphilipnienhuisAttached File-=>Added strread.patch, #24315
      StatusIn Progress=>Patch Submitted
      Carbon-Copy-=>Added bpabbott
    Thu 03 Nov 2011 08:24:33 PM UTCphilipnienhuisAttached File-=>Added provsnl_strread.m.diff, #24297
    Thu 03 Nov 2011 05:21:54 PM UTCphilipnienhuisStatusNone=>In Progress
      Assigned toNone=>philipnienhuis

    Back to the top


    Powered by Savane 3.1-cleanup