bugGNU Octave - Bugs: bug #37961, textscan : whitespace...

 
 

bug #37961: textscan : whitespace specification doesn't work

Submitted by:  None
Submitted on:  Wed 19 Dec 2012 06:25:57 PM UTC  
 
Category: InterpreterSeverity: 2 - Minor
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: Philip Nienhuis <philipnienhuis>
Originator Name: maf88_Originator Email: -unavailable-
Open/Closed: ClosedRelease: 3.6.3
Operating System: GNU/Linux

Add a New Comment(Rich 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)

Wed 02 Jan 2013 04:33:33 PM UTC, comment #19:

Thanks. Those two lines were all this bug report needed:

http://hg.savannah.gnu.org/hgweb/octave/rev/878623ca15e1

As to the merge issues, they are trivial:

http://hg.savannah.gnu.org/hgweb/octave/rev/1733bd181cb6

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Sun 30 Dec 2012 09:04:35 PM UTC, comment #18:

Well, it may not be so very super-easy.

(1) Wouldn't such a fix, on top of the 5 Ben mentioned, interfere with the actual fix from almost a year ago (changeset 14213:a022c04f68cc by Rik) once those 5 are finally merged into the next stable release?

(2) Simply (pre-)defining the variable "whitespace" isn't the way to go as that will induce many obscure problems later on in strread.m
Luckily the actual fix boils down to replacing L. 114
args{ipos+1} = unique ([" ", whitespace]);
with
args{ipos+1} = unique ([" ", args{ipos+1}]);

...plus adding a ";" at the end of L.109
(all relative to the Octave-3.6.3 sources)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 30 Dec 2012 04:46:47 PM UTC, comment #17:

As far as I can see, the actual critical bug on stable is quite easy to fix: just define the whitespace variable before the codepath in which it used undefinedly. It may be easier to just make that small fix.

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Sun 30 Dec 2012 04:34:03 PM UTC, comment #16:

Jordi, ignoring the style, deprecation, and spell-check changes, there are five changesets (below) that impact the results. Do you mean to imply that adding all of these is too aggressive? I tend to think it is, but am worried that a subset might introduce unexpected behavior.

Ben Abbott <bpabbott>
Project Member
Sun 30 Dec 2012 04:18:48 PM UTC, comment #15:

Philip, that's a lot of changes:

We need to be a lot more conservative than this on the stable branch. We can't just copy over all of the changes from default. Do you have any indication which one(s) of these fix the current issue on stable?

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Fri 28 Dec 2012 10:07:22 PM UTC, comment #14:

Oopsie - I should have written: all csets since Octave 3.6.0 was released

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Fri 28 Dec 2012 04:10:24 PM UTC, comment #13:

Obviously all csets on the files textscan.m / strread.m / textread.m since the 3.6.3 release (there weren't so many).

Tonight & tomorrow I have no time, maybe Sunday I can have a look to sort out what csets are involved.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Fri 28 Dec 2012 02:28:19 AM UTC, comment #12:

The bug is still open in stable. There should be no codepath that produces a syntax error. Please do not close this bug report until we backport the correct csets back into stable. Any suggestions on what those csets are?

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Thu 27 Dec 2012 11:14:23 PM UTC, comment #11:

Your analysis is spot-on:

while Octave with 3.7.0+ textscan/strread gives:

There is something fishy in Octave but I cannot easily see where the first warning comes from. I've fixed the second warning to be more clear (http://hg.savannah.gnu.org/hgweb/octave/rev/4294430ef736).
Anyway the comments (that I added long time ago) in strread.m around L.600 do confirm your analysis: when using a "%s" format you can't expect Octave, or ML, to reliably distinguish the corresponding text field from an immediately following literal.

That, plus different results in r2012b than in r2013a for your second call is adequate to close this bug with "Invalid".

BTW your patch does give some useful hints on how to process repeated whitespace in %s fields. If I get time & need it myself, and jwe doesn't beat me with textcan.cc, I might have another look at it.

Let's leave it to Jordi to decide if he wants a more robust strread/textscan/textread for upcoming 3.6.4.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 27 Dec 2012 09:13:45 PM UTC, comment #10:

Looking again, I see the OP's syntax is clumsy/ugly. If the whitespace parameter is dropped then the statement below is in error.

The first "%s" will read the entire string. The will be no semicolon remaining to match the "literal" one and nothing remaining to be read by the second "%s".

The OP's full syntax does work for me with MatlabR2012b

While I don't see it in Mathworks documentation, it does appear that the format string is parsed while respecting the "whitespace" parameter. However, for the format string the characters " \t" are also respected.

I agree we shouldn't try to copy this "feature", as it leads to confusing syntax (like the OP's).

Ben Abbott <bpabbott>
Project Member
Thu 27 Dec 2012 04:10:23 PM UTC, comment #9:

Ben,

There may be a few issues with your changeset, sorry.

(A) I think the first hunk (concerning eol_char) is unwanted. Variable 'eol_char' was only introduced to process the "endofline" parameter for textscan/textread.
strread.m should have nothing to do with explicit EOLs unless:
- called by textread or textscan
- when the user specifies "endofline" (in fact a textscan/textread parameter)
In all other cases "\n" is part of white space as defined in the ML docs (" \b\t\r\n"), and as such is implicitly a delimiter.

(B) (I promise it's the last time I mention this) AFAICS Matlab has no documentation for what happens if there's a literal in a format string that is simultaneously defined as whitespace. I would be hesitant to "fix" such a "bug". See (C) below...

FYI, Test #10 in strread.m is an example of reading white space with %s format conversion specifiers. It boils down to essentially setting whitespace empty and explicitly specifying delimiter(s). That test is based on older ML docs and examples.

(C) In newest ML r2013a (prerelease), the OP's problem (with the text in his original report and his invocation of textscan) gives:

... so as his examples no longer works in newest ML I even doubt if ML compatibility is a valid issue here. (A fine example of why it is unwise to rely on undocumented ML behavior)

Again, I think the OP simply invoked textscan (1) in a clumsy way with (2) unfortunately an old and less robust version of the textscan/strread/textread trio.

(D) The newest on-line ML docs (r2012b) for strread can even be interpreted as that whitespace MAY only be one of \b \n \r \t \\ \% "'" (intriguingly space itself isn't mentioned, it used to be in older ML docs).

To Jordi:
Yes I don't see any reason not to apply the most recent textscan/textread/strread versions to stable (i.e., for the 3.6.4 release).

BTW we're still waiting for the OP's answers.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Tue 25 Dec 2012 04:34:23 AM UTC, comment #8:

I've attached a changeset (somewhere between ugly and clumsy). All tests for strread() pass. A test needs to be added to capture the problem reported here.

I've also attached a script that tests for the reported problem, when strread() is used instead of textscan().

There remains a problem with textscan.

(file #27149, file #27150)

Ben Abbott <bpabbott>
Project Member
Tue 25 Dec 2012 03:44:51 AM UTC, comment #7:

Running either Octave's default or stable branch, the original example gives the error below.

Matlab gives the same result for FMT = "%s;%s" and FMT = "%s %s". It is the first that gives produces the error for Octave. Matlab's result is below.

WIth FMT = "%s %s", neither Octave's stable and default branch throws an error, but each return the same incorrect result.

It looks to me as if the problem is with strread(). Looking over strread(), the problem seems to due to two features of the implementation.

  1. trread() ultimately converts all "whitespace" to char(32). This will lead to problems reading strings that should include spaces.
  2. The current implementation also trims the input string of spaces in a few locations.

The code responsible for parsing the string is complex and difficult for me to follow. There may be other problems there.

Ben Abbott <bpabbott>
Project Member
Mon 24 Dec 2012 05:59:34 PM UTC, comment #6:

The point is that there is a bug on stable that is not on default. If the bug is minor enough, fine, we can ignore it, but otherwise, we should backport the fix from default into stable.

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Thu 20 Dec 2012 09:20:03 PM UTC, comment #5:

Just to get back to the original bug report:
I don't see why a 'whitespace' parameter is used to essentially specify a delimiter.

In both Octave (dev version) and ML r2012a the following works OK on the example:

A = textscan (fid, "%s %s", "Whitespace", "", "delimiter", ";")

Note that

B = textscan (fid, '%s %s %s', 'Whitespace', ';')

won't work on the example as Octave cannot substitute empty values for fields beyond an EOL on incomplete lines (L.1 contains just two fields). A compiled textscan() could...
A way around that is to invoke textscan in two (or more) passes, each time with a different format string, + format repeat count (1st pass) or 'headerlines' param (last pass) or both (intermediate passes, if present).

Ben, as to your observation:
<QUOTE>
whitespace=";" implies that spaces, tabs and such are not whitespace.
</QUOTE>
Correct. The default for whitespace (" \b\t" is replaced by a user-specified value, as it should.
But.... in L.s 124..125 of textscan.m, it also reads:
## If there is a string format AND whitespace value = empty,
## don't add a space (char(32)) to whitespace
This is based literally on TMW's on-line help for textscan.m. (see whitespace section).

While I do think textscan.m's (or rather strread.m's) whitespace processing can be improved, AFAICS the setup of the call to textscan by the OP was faulty / clumsy in the first place.

I tend to close this bug report with (in decreasing order of preference):
"Invalid" (faulty call to textscan), or
"Works for me" (= with proper call to textscan), or
"Fixed" (as it relates to an old textscan/strread version), or
"Won't fix" (I can't really guarantee that ;-) )
Who takes the pick?

I'll decrease priority in any event.

Off-topic:
I just noted that TMW changed the behavior of textscan: specifying multiple delimiter chars is now supposed to be in a cell array rather than as a character string.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 20 Dec 2012 03:24:35 AM UTC, comment #4:

Matlab gives;

I had expected the proper syntax to read the file to be;

However, the results from Matlab do not match Octave. It appears that whitespace=";" implies that spaces, tabs and such are not whitespace.

Ben Abbott <bpabbott>
Project Member
Wed 19 Dec 2012 08:20:02 PM UTC, comment #3:

Sure, but OP refers to 3.6.3.

I can't track 'undefined whitespace' around L.114 in the dev version of textscan.m, so that 'whitespace undefined' bug has been fixed already. Perusing the file log I'd say probably > 2 years ago (when was 3.6.0 (last stable version) released?)
Similarly for a missing ';' at L.109.

Anyway I can't reproduce OP's problems in the dev version. I get output but no errors using his files, but obviously I cannot judge if it is good or not. I think not as I get numbers rather than strings. Let the OP give us relevant info.

My point about ';'-literal and defining it as whitespace still stands.

To the OP (maf88_): advice: grab textscan.m, textread.m and strread.m (they are closely related) from here:
http://hg.savannah.gnu.org/hgweb/octave/file/1e9a6285acc4/scripts/io
and swap them for the versions you have in /usr/local/share/octave/3.6.3/m/io/
,try again and report back.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Wed 19 Dec 2012 07:18:45 PM UTC, comment #2:

The bug is genuine, there is a whitespace variable in this code path that isn't defined here. Looks like a cut-and-paste error.

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Wed 19 Dec 2012 06:59:33 PM UTC, comment #1:

Try with small caps ('whitespace') and report back, pls.
textscan's / strread's / textread's parameters are case-sensitive.

BTW having a "literal' ';' between the format conversion specifiers and at the same time declaring it as whitespace may lead to unpredictable results. What does Matlab return?

Also note that your first line will come out differently from the rest. Use 'headerlines' parameter? delimiters will implicitly be ' \n' (space & newline)

The fmt echo is stray, I'll look into this.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Wed 19 Dec 2012 06:25:57 PM UTC, original submission:

Hello,

The file i want to parse :
XQPCTick;Tag
1552111181823;G8 Throw
1552327767233;G8 Throw
1552570758808;G8 Throw
1552819036603;G8 Throw
1553037382424;G8 Throw
1553238114161;G8 Throw
1553456459982;G8 Throw
1553687128680;G8 Throw
1553905469514;G8 Throw
1554100925005;G8 Throw

The function I use

A=textscan(fid,'%s;%s', 'Whitespace', ';');

And the error I get:

The error i get :

fmt =
{
[1,1] = %s;
[1,2] = %s
}
`whitespace' undefined near line 114 column 37
error: called from `textscan' in file /usr/local/share/octave/3.6.3/m/io/textscan.m near line 114, column 21
error: called from:
error: /usr/local/share/octave/3.6.3/m/miscellaneous/run.m at line 55, column 7

Anonymous

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #27149:  changeset.patch added by bpabbott (2KiB - application/octet-stream)
file #27150:  go_37961.m added by bpabbott (1KiB - application/octet-stream)
file #27127:  load_tagstream.m added by None (1KiB - text/x-objcsrc - The code (not mine though).)
file #27128:  P3_2_8_p29.csv added by None (987KiB - text/csv - The code (not mine though).)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 02 Jan 2013 04:33:33 PM UTCjordighStatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Fri 28 Dec 2012 02:28:19 AM UTCjordighStatusInvalid=>Confirmed
      Open/ClosedClosed=>Open
    Thu 27 Dec 2012 11:14:23 PM UTCphilipnienhuisStatusPatch Submitted=>Invalid
      Open/ClosedOpen=>Closed
    Tue 25 Dec 2012 04:34:23 AM UTCbpabbottAttached File-=>Added changeset.patch, #27149
      Attached File-=>Added go_37961.m, #27150
      StatusConfirmed=>Patch Submitted
    Thu 20 Dec 2012 09:20:03 PM UTCphilipnienhuisSeverity3 - Normal=>2 - Minor
    Wed 19 Dec 2012 07:18:45 PM UTCjordighStatusNeed Info=>Confirmed
    Wed 19 Dec 2012 06:59:33 PM UTCphilipnienhuisStatusNone=>Need Info
      Assigned toNone=>philipnienhuis
    Wed 19 Dec 2012 06:25:57 PM UTCNoneAttached File-=>Added load_tagstream.m, #27127
      Attached File-=>Added P3_2_8_p29.csv, #27128

    Back to the top


    Powered by Savane 3.1-cleanup1