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
|
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)
|
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.
|
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.
|
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?
|
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
|
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.
|
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?
|
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.
|
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).
|
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.
|
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)
|
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.
- trread() ultimately converts all "whitespace" to char(32). This will lead to problems reading strings that should include spaces.
- 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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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
|