bugGNU Octave - Bugs: bug #33876, textscan: resuming reading does...

 
 

bug #33876: textscan: resuming reading does not work

Submitted by:  Philip Nienhuis <philipnienhuis>
Submitted on:  Thu 28 Jul 2011 06:24:17 AM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: FixedAssigned to: Ben Abbott <bpabbott>
Originator Name: Philip NienhuisOpen/Closed: Closed
Release: 3.4.2Operating 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)

Sat 22 Jun 2013 02:56:39 AM UTC, comment #17:

Even if this isn't fixed properly, both textscan and strread have changed significantly in the last two years. If there is a similar bug, it is best to start a new report so that comments regarding the old implementation don't confuse anyone.

Besides, this report was for 3.4.2.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Sat 22 Jun 2013 02:35:46 AM UTC, comment #16:

Can this bug be closed now? It appears that a patch was committed 2 years ago to add resuming reading to textscan().

Rik <rik5>
Project Administrator
Thu 04 Aug 2011 12:06:59 AM UTC, comment #15:

I pushed the change.

http://hg.savannah.gnu.org/hgweb/octave/rev/5d0478a75618

There remain some inconsistencies with ML. See comment #5.

https://savannah.gnu.org/bugs/?33876#comment5

I'll have to study ML's docs further before I'm sure ML is behaving as it should.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Wed 03 Aug 2011 11:58:25 PM UTC, comment #14:

All the integrated tests pass. However, resuming reading still fails.

I'll push a changeset to fix this.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Wed 03 Aug 2011 11:13:50 PM UTC, comment #13:

oppps ... I ran the tests with the old textscan. I'll start over.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Wed 03 Aug 2011 11:08:38 PM UTC, comment #12:

I took a quick look. I see a test failure for textscan

Matlab gives a different result

Is this expected?

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Wed 03 Aug 2011 10:16:55 PM UTC, comment #11:

I got some time and pushed Philip's patches today in this changeset:

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

Feel free to update to the latest tip and apply your additional patches.

Rik <rik5>
Project Administrator
Wed 03 Aug 2011 03:49:30 PM UTC, comment #10:

Ben, Phillip,

I will try and review and push Philip's patches this Friday or Saturday (8/5 or 8/6).

Rik <rik5>
Project Administrator
Mon 01 Aug 2011 11:53:44 AM UTC, comment #9:

Rik. I'll wait on Philp's patches. I just want to be sure that we don't delay until after the next release.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Mon 01 Aug 2011 01:17:02 AM UTC, comment #8:

I will commit Philip's patches in a few days. Is it possible to wait until after that? I'd like the patches to apply cleanly to the Mercurial code base.

Rik <rik5>
Project Administrator
Sun 31 Jul 2011 09:18:21 PM UTC, comment #7:

I also think that textscan() eventually needs to be rewritten. But I'm also inclined to patch the current version so that it works as intended.

If there are no objections, I'll push the changeset.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Sun 31 Jul 2011 09:04:32 PM UTC, comment #6:

Attached is a tiny patch for strread.m needed in some corner cases, I hit one exactly when trying resuming reading in the course of other issues. Perhaps it helps...
(This patch forms part of a bigger patch set that I sent privately to Rik a few days ago and that covered CollectOutput, additional fixes for MultiDelimsAsOne, int32 casts, + revised tests because of the int32 output for %d %u arguments.)

Oh, in your first script there's a syntax error in the one but last isequal (missing "}")

Without going further into the details of your patches: as regards your ML findings, I'd like to mention that I find Matlab's behaviour often inconsistent and unpredictable. That applies especially for textread and strread (which they apparently are going to drop in the future), but textscan is not very clear either. Or at least the ML docs aren't.
Sometimes there are two or more seemingly equivalent but different format strings of which just one works (in "difficult" files, admittedly), sometimes dependent on extra parameter values.

As to your patch: those refer to parts of textscan.m that I didn't touch - Rik made those changes. From the header in the message you sent through the bug tracker I see he's not in the mail notification list - I think he should be so I added him.

Apart from the multi-line format string issue I mentioned earlier it occurred to me there's another gotcha in the way format repeat count currently works in textscan/textread:
The potential existence of comment lines in the data is completely ignored.
So you specify using a format string N times expecting to read a specific amount of data, and in extreme cases you get back an empty output because the first N lines were actually comment lines.

Therefore I'm not sure if much more effort should be invested in the current situation. Maybe only as a temporary fix.

My ideas on this issue:

(A) The overall superior way would be to have a compiled strread. That could plough linearly through the file, rather than the forced column-wise way it works now. But yes this is one of those nasty "if only...."s.

(B) Starting from the current state of things, I think a better way would be to:
(1) Re-implement format-repeat count processing in strread;
(2) In case of format repeat count arguments, have textread and textscan just read a liberally big chunk of the file (or string);
(3) Let textread and textscan communicate to strread that a format repeat count was requested (by some non-documented parameter);
(4) Let strread sort out how much of the chunk was actually needed;
(5) Let strread communicate back to textscan or textread how much was read of the data text string it was passed (a non-documented output arg);
(6) Have textscan or textread do an fseek using the info they got back from strread.

- This would be a more stable fix that covers all known cases;
- Format repeat count would work again for strread (ML compatibility);
- Textscan could resume from either files or text strings (now it only "works" for files; => better ML compatibility as well).

But especially step (4) needs good analysis - the input string is completely mangled, stripped, split up, recombined and stripped and split again before strread knows how many "format string occurences" were actually present in the data. Keeping track of the exact limit through all these steps can be complicated ("can" - I haven't looked at it in detail).

To do this, textscan and textread may need to know a bit more exactly how much to read.
To do this cleanly, I think some parts of strread have to be separated into separate utility functions in a ./private directory. These can be called by textscan & textread, so some identical code appearing in all three could be dropped.
This refers to the format string parsing section and the part where the format stuff is matched to the first "line" of the file (or text string).
Perhaps comment line processing may also come in the picture.

I don't doing this, but I have no idea when I will have time for this. Now that textscan/textread/strread work acceptably well, I'd rather help building the 3.4.x MinGW binary.

As far as John's plans go, thank you for pointing me onto that.
I'll post in the maintainers list about it.

(file #23719)

Philip Nienhuis <philipnienhuis>
Project Member
Sun 31 Jul 2011 05:57:38 PM UTC, comment #5:

Philip, thanks for the summary.

I thought I should mention that jwe plans to work on this part of the code, But on a different problem. I think it is the same problem reported in the bug here -> https://savannah.gnu.org/bugs/?33875

Oct 2010
Thread: advice / help needed for reading formatted text (textscan, strread, & textread)
https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021513.html

I wrote another test script to help me understand how ML's implementation of textscan() parses files. The isequal() checks each return true for ML.

The patch I've attached (the 2nd one) functions correctly the first time the file is read (the developers version does not).

The second and third examples have failures. In the second example, ML's result reads "0" for c2{2}(1,1). Looking at the contents of fubar.txt ...

The "0"'s in both the 2nd and 3rd examples occur when textscan reads beyond an EOL.

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Sun 31 Jul 2011 11:57:51 AM UTC, comment #4:

Ben (just a quick but long-winded note),

Rik and I had a fair bit of email exchange behind the scenes lately about textscan/textread/strread where quite a few details were discussed, so you probably missed a few things.

What happens in textscan with the current implementation is that rather than exactly repeating a format, reading a line (file record) is repeated instead. That is a subtle but vital difference.
I.e,. it is assumed that a format string always applies to one "line" in the file.
This breaks when a format applies to several "words" / "fields" / "items" (whatever you call these) spread over multiple lines.

E.g., I have several FORTRAN output files that look like:

:
465123.56 112345.32 83 83.44 76.33
15 456.22 0.0123 9 320.01 -1.709
:

from which I can read using something like

textscan (fid, "%f%f%d%f%f%d%f%f%d%f%f", delimiter, ' ')

Furthermore, it is allowed to call textread and strread using:

[arg1, arg2, arg3, arg4] = textread (file)

(i.e. with a default format of "%f")

-or-

[arg1, arg2, arg3, arg4] = textread (file, "%f", N)

-or even-

[arg1, arg2, arg3, arg4] = textread (file, "", N)

where the number of output arguments is determined by the user).

So your (and Rik's) patch work only of a format string applies to just one file "line" (record).
This not exactly equivalent to "repeating a format" as it is mentioned in the on-line ML docs.

If your patch works (can't check right now, maybe tonight) we can always add a note in the texinfo header that for "multi-line formats" N has to be multiplied by the nr. of lines / format.

BTW format repeat count doesn't work in strread itself (see below).

A bit of background:

Of course, in principle anything can be fixed, but it may become very complicated with the current strread implementation (that I simple accepted and used as a given) to do this for format repeat count.

That's because only after the file and format string have been fully parsed and deciphered by strread (including literals bordering fields w/o delimiters in between, and including format specifiers with trailing literals) it is known for sure how many fields are contained in one file "line".
Strread (1) simply counts the number of format fields (incl. literals and %* specifiers), then (2) splits the file into "columns" of words, where the number of columns matches the number of format items, and (3) finally transforms the columns into the requested format. Somewhere in step (2) the concept of file line is dropped in favour of a number of fields matching the number of format items.

Only in above step (3) of strread it would be possible to exactly return the number of words matching a specific format repeat count.
Initially I had that in place but Rik dropped it and implemented the current setup, with good reason (it avoids reading the entire file, which I overlooked).

BTW I just note that having it moved out of strread has the side effect that format repeat count doesn't work anymore in strread itself... (so Rik and I both overlooked that one :-( )

I do have an idea of how to fix this situation, using some communication between strread and textscan/textread) but currently I have other priorities.

BTW, I sent Rik some more patches a few days ago (small bug fixes + CollectOutput option).

Philip Nienhuis <philipnienhuis>
Project Member
Sat 30 Jul 2011 11:57:03 PM UTC, comment #3:

The first changeset attempt didn't handle the EOF correctly. I've made second.

(file #23717)

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Sat 30 Jul 2011 09:23:57 PM UTC, comment #2:

I've attached a patch. When I get a confirmation that I've fixed the reported bug, I'll push it.

(file #23716)

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Sat 30 Jul 2011 06:17:04 PM UTC, comment #1:

Coincidently, I just wrote a script that I'd thought was working correctly. After see the bug report, I checked and found it was not.

Below is a simple script I used to check (using the developer's sources)

and the result was ...

It looks like each time textscan is called, and extra line is read, but not processed.

Philip, can you confirm this is the same problem you've reported? ... or is there something more?

Ben Abbott <bpabbott>
Project MemberIn charge of this item.
Thu 28 Jul 2011 06:24:17 AM UTC, original submission:

(known issue, entered so we don't forget)

Textscan does not remember where it "stopped reading" when format repeat specifiers have been specified and the same fid or text string variable is to be read from.

Resuming reading from there doesn't work.

Philip Nienhuis <philipnienhuis>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #23719:  strread.m.diff_30July2011 added by philipnienhuis (387B - application/octet-stream)
file #23717:  changeset.patch added by bpabbott (810B - application/octet-stream - 2nd attempt at a changeset)
file #23716:  changeset.patch added by bpabbott (755B - application/octet-stream - 1st attempt at a changeset)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by philipnienhuis
  • -unavailable- added by bpabbott (Posted a comment)
  • -unavailable- added by philipnienhuis (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 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 22 Jun 2013 02:56:39 AM UTCbpabbottStatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Sun 31 Jul 2011 09:04:32 PM UTCphilipnienhuisAttached File-=>Added strread.m.diff_30July2011, #23719
      Carbon-Copy-=>Added -unavailable-
    Sat 30 Jul 2011 11:57:03 PM UTCbpabbottAttached File-=>Added changeset.patch, #23717
    Sat 30 Jul 2011 09:23:57 PM UTCbpabbottAttached File-=>Added changeset.patch, #23716
      StatusNone=>Patch Submitted
      Assigned toNone=>bpabbott

    Back to the top


    Powered by Savane 3.1-cleanup