patchGNU Octave - Patches: patch #8783, C++ implementation of textscan

 
 

patch #8783: C++ implementation of textscan

Submitted by:  None
Submitted on:  Mon 26 Oct 2015 05:09:50 AM UTC  
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Lachlan Andrew <lachlan> Originator Email:  -email is unavailable-
Open/Closed:  Closed

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)

Sat 19 Mar 2016 12:12:27 PM UTC, comment #108: 

Thanks Mike.

Ben Abbott <bpabbott>
Project Member
Sat 19 Mar 2016 03:04:07 AM UTC, comment #107: 

Ok, I added to another test based on your additions and all looks good to me, pushed to default (under Lachlan's name from the original patch).

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

Mike Miller <mtmiller>
Project Administrator
Sat 19 Mar 2016 02:30:44 AM UTC, comment #106: 

I've updated the changeset and modified a test.

(file #36694)

Ben Abbott <bpabbott>
Project Member
Sat 19 Mar 2016 01:51:28 AM UTC, comment #105: 

Ok, with your patch applied, the example you posted does pass for me, and all unit tests are still passing. Am I missing anything? Should we add this example as another test?

Mike Miller <mtmiller>
Project Administrator
Sat 19 Mar 2016 01:16:45 AM UTC, comment #104: 

Mike

The correct behavior is below

>> c = textscan (sprintf ('1 2 3\n4 s 6\n'), '%f %f %f', 'ReturnOnError', 1)
c =
{
  [1,1] =

     1
     4

  [1,2] =  2
  [1,3] =  3
}

I think I've figured how to update the changeset. The modified version is attached (my c++ skills are very poor). The tests still need to be corrected.

(file #36693)

Ben Abbott <bpabbott>
Project Member
Sat 19 Mar 2016 01:06:37 AM UTC, comment #103: 

Ben, now that textscan is in core, it might be clearer to start a new bug report about any outstanding problems you are having with it.

As it stands in the default branch right now, all tests as they are currently written pass for me. Not following this whole thread, nor really wanting to dig into it, it's not clear to me what file #36680 is supposed to address. I took a look at it, and the changes to the unit tests have already been applied by Philip in http://hg.savannah.gnu.org/hgweb/octave/rev/1b48a5d04fb1, but not the code change.

Mike Miller <mtmiller>
Project Administrator
Sat 19 Mar 2016 12:43:07 AM UTC, comment #102: 

More changesets have been pushed some my last comment.

c = textscan (sprintf ('1 2 3\n4 s 6\n'), '%f %f %f', 'ReturnOnError', 1)

c =
{
  [1,1] =

     1
     4

  [1,2] =

       2
     NaN

  [1,3] =

       3
     NaN

}

An updated version of file #36680 is needed.

Ben Abbott <bpabbott>
Project Member
Fri 18 Mar 2016 12:26:26 PM UTC, comment #101: 

@Lachlan,

After a fresh pull, file #36680 no longer applies.

$ patch -p1 < ../textscan/textscan-return_on_error.cset
patching file libinterp/corefcn/textscan.cc
Hunk #1 succeeded at 1031 (offset -3 lines).
Hunk #2 FAILED at 2905.
Hunk #3 FAILED at 3143.
Hunk #4 FAILED at 3159.
3 out of 4 hunks FAILED -- saving rejects to file libinterp/corefcn/textscan.cc.rej

Ben Abbott <bpabbott>
Project Member
Fri 18 Mar 2016 12:21:49 PM UTC, comment #100: 

@Lachlan with file #36680 all tests pass.

test textscan
PASSES 76 out of 76 tests

And ReturnOnError now produces empties (as ML does)

>> c = textscan (sprintf ('1 2 3\n4 s 6\n'), '%f %f %f', 'ReturnOnError', 1)
c =
{
  [1,1] =

     1
     4

  [1,2] =  2
  [1,3] =  3
}

Ben Abbott <bpabbott>
Project Member
Fri 18 Mar 2016 11:26:28 AM UTC, comment #99: 

One last post here: ("famous last words")

I've fixed the 3 failing tests after making sure textscan.cc does the same as Matlab 2016a, pushed here:
http://hg.savannah.gnu.org/hgweb/octave/rev/1b48a5d04fb1

Only one test from old textscan.m FAILS but that is a far-fetched test where Matlab doesn't even manage to read beyond the first line:

%! ## No delimiters at all besides EOL.  Skip fields, even empty fields
%! str = "Text1Text2Text\nTextText4Text\nText57Text";
%! c = textscan (str, "Text%*dText%dText");
%! assert (c{1}, int32 ([2; 4; 0]));

Let's forget this test.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 18 Mar 2016 09:47:00 AM UTC, comment #98: 

Lachlan, others,

I'm afraid textscan.cc isn't done yet completely :-)
Running "test textscan.m" (so that textscan.cc does the work in textscan.m's tests) yields ~10 FAILs; and on Linux and Windows (crossbuild completes now) I now saw 3 FAILS with textscan.cc; 2 of them already mentioned in comments 84-88, a new one relating to ReturnOnError.
The FAILs with textscan.m's tests could imply regressions, but before drawing conclusions I'll have a better look later on.

What I'll do: I'll close this patch report now, and open a new bug report for the FAILs once I have a good overview of them (hopefully tonight or tomorrow).
I'll finish strread.m + optionally textread.m later, no need for a bug/patch report for that I think.

Thanks Lachlan for your patience with me. Comment #8 is very valid.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 18 Mar 2016 03:26:42 AM UTC, comment #97: 

The attached changeset fixes the anomaly Ben pointed out.  Now a parse error causes the output to be truncated for either a file or a string.

Thanks to Philip for his tireless help in this, and thanks to everyone else for polishing the many rough edges.  It has been fun and I've learned a lot, but will be glad to put this patch behind me :)

(file #36680)

Lachlan Andrew <lachlan>In charge of this item.
Fri 18 Mar 2016 12:56:53 AM UTC, comment #96: 

Sorry for delaying posting the update in response to Philip's request.  I just wanted to check the update one more time...

One of the changes I made was eliminating the difference between character and file input, so I'll see if that fixes Ben's problem.

Lachlan Andrew <lachlan>In charge of this item.
Fri 18 Mar 2016 12:11:40 AM UTC, comment #95: 

@All, I'm able to build from a fresh archive.

However, I see some unexpected behavior (Mac OS X). textscan does not give consistent results when using character and file input.

c = textscan (sprintf ('1 2 3\n4 s 6\n'), '%f %f %f', 'ReturnOnError', 1)
c =
{
  [1,1] =

     1
     4

  [1,2] =

       2
     NaN

  [1,3] =

       3
     NaN

}

>>  f = tempname ();
>>  fid = fopen (f, "w+");
>>  fprintf (fid, "1 2 3\n4 s 6\n");
>>  fseek (fid, 0, "bof");
>>  c = textscan (fid, "%f %f %f", "ReturnOnError", 1);
>>  fseek (fid, 0, "bof");
>>  fclose (fid);
>>  unlink (f);
>> c
c =
{
  [1,1] =

     1
     4

  [1,2] =  2
  [1,3] =  3

Using Matlab, I get a consistent result.

c = textscan (fid, '%f %f %f', 'ReturnOnError', 1)

c =

    [2x1 double]    [2]    [3]

>> c = textscan (sprintf ('1 2 3\n4 s 6\n'), '%f %f %f', 'ReturnOnError', 1)

c =

    [2x1 double]    [2]    [3]

Ben Abbott <bpabbott>
Project Member
Thu 17 Mar 2016 09:27:54 PM UTC, comment #94: 

@JWE:
Thanks, my pleasure. Nice to see that the slow textscan.m and large parts of strread.m can be retired (I'll be working on weeding out strread.m next).
I'm just building now, hopefully the cross-build issues I met (comment #90) will be gone.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 17 Mar 2016 09:20:47 PM UTC, comment #93: 

I fixed a problem with a build circular dependency here:

http://hg.savannah.gnu.org/hgweb/octave/rev/6e5540ddb639

I'm not sure if this was needed in an earlier revision of this patch, but it doesn't seem to be needed now and in fact caused compilation errors due to build system dependencies.

I wasn't paying attention to this patch discussion, but I see this was brought up before.

Mike Miller <mtmiller>
Project Administrator
Thu 17 Mar 2016 08:18:40 PM UTC, comment #92: 

Lachlan and Philip, thanks for all the work on this function.

I applied the latest changeset to the Octave sources and pushed it here:

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

Running the tests caused a strange crash for me (double free when calling the destructor for textscan even though there is no explicitly defined destructor for that class) which I think I tracked down to an out of bounds assignment to delim_table.  I avoided that in the following changeset, though I'm not sure this is the correct fix (why -2):

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

While trying to understand why the crash was happening, I ran the textscan tests with valgrind.  I'm attaching a log of what I see.  Search for "Invalid read size of 1" and you'll see that there seem to be some places where arrays or strings are accessed out of bounds.  I'm not sure what the best fix is, but these need to be cleaned up.

(file #36677)

John W. Eaton <jwe>
Project Administrator
Thu 17 Mar 2016 04:55:21 PM UTC, comment #91: 

Same problem with JOBS=1

Philip Nienhuis <philipnienhuis>
Project Member
Thu 17 Mar 2016 07:56:46 AM UTC, comment #90: 

Lachlan, John:
When cross-building with the patches in files 36634 and 36643, the build consistently breaks here (still same as in comment #86):

/home/philip/devel/octdev/mxe/mxe_w64_20150316/tmp-default-octave/octave-4.1.0+/.build/libtool: line 10095: /home/philip/devel/octdev/mxe/mxe_w64_20150316/tmp-default-octave/octave-4.1.0+/.build/src/octave: No such file or directory
Makefile:26970: recipe for target 'doc/interpreter/doc-cache' failed
make[5]: *** [doc/interpreter/doc-cache] Error 127
make[5]: *** Waiting for unfinished jobs....

Took me a bit of sorting out (recloning & building both Octave and mxe-octave, trying) to be sure it were the textscan patches that provoked this, sorry for that delay.

(BTW I usually build with JOBS=5. I'll have another try with JOBS=1)

Philip Nienhuis <philipnienhuis>
Project Member
Thu 17 Mar 2016 02:11:01 AM UTC, comment #89: 

@Lachlan, regarding the build process and builtin-defun-decls.h. It appears that if textscan.cc was in directory dldfcn instead of corefcn it would build without error (i.e. other cc files in dldfcn also depend upon builtin-defun-decls.h).

Ben Abbott <bpabbott>
Project Member
Wed 16 Mar 2016 07:55:29 AM UTC, comment #88: 

OK I yield....
But then something must be done about the 2nd FAIL, either the test or the code. Your pick?

Maybe it is time (for me) to run all the textscan/strread tests through Matlab again. After all, only now that the limitations of the m-code have vanished we can strive for max compatibility. I'm not pessimistic BTW, I think at most we'll only catch rare corner cases like bug #44750.
Trying the example there in Matlab r2016a learns that w/o multipledelimsasone=1 we get {'', 'home','foo'} and with it we get {'home 'foo'}. Intriguing but I suppose your reasoning is better than mine. Your observation that no design choice can be 100 % consistent is absolutely right.

Looking back I've learned a lot about Octave's m-code figuring out how to reliably parse complicated repeated text patterns. But m-code was never the best means for what textscan is to get together. I'm happy with leaving the role of strread/textread/textscan custodian behind me :-)

Philip Nienhuis <philipnienhuis>
Project Member
Wed 16 Mar 2016 02:48:51 AM UTC, comment #87: 

Philip,

Ignoring the leading commas in the second FAIL seems quite consistent.  The end-of-line (which is an implicit delimiter) is merged with other delimiters when MultipleDelimsAsOne is specified, and not merged when that is not specified.

Ignoring leading delimiters on the first line is less clear.  However, that was done to pass the test in textscan.m that was introduced for bug #44750, which was parsing "/home/foo/" into {"home", "foo"} (rather than {"", "home", "foo"}).

Also, remember that you expected the behaviour on the first line to match the behaviour on subsequent lines.  To achieve that while merging EOL-delimiters with other delimiters, it is necessary to ignore leading delimiters on the first line.  Is it a bug?  I'd say that no design choice is 100% consistent in this case, and that TMW's is as reasonable as any.

However, you are the custodian of textscan: if you want leading delimiters on the first line to count, that saves us two lines of code :)

Lachlan Andrew <lachlan>In charge of this item.
Tue 15 Mar 2016 11:55:50 PM UTC, comment #86: 

In the first FAIL it is really just a wrong test. Matlab and Octave behave exactly the same. I just added that comma to the text string and the FAIL was gone (also Matlab gave the "right" answer).

I'm afraid that the second FAIL is a case of "bug-for-bug" Matlab compatibility. Would you agree that a line like
",,,,10" does have a missing (empty) field before the first comma?
If multipledelims = 0, Octave and Matlab both recognize the empty field.
If multipledelimsasone = 1, suddenly that empty field isn't recognized anymore.  IMO that is inconsistent, even if it is the supposedly "proper" behavior.
Well, let JWE have the final word here.
I will mention this "bug" on the Matlab prerelease feedback web page; I'm curious as to what TMW will say about it (if they react at all).

My mxe cross-build errors out in the end with:

libtool: install:  /usr/bin/install -c libinterp/.libs/liboctinterp-3.dll /home/philip/devel/octdev/mxe/mxe_w64_20160308/tmp-default-octave-install/home/philip/devel/octdev/mxe/mxe_w64_20160308/usr/x86_64-w64-mingw32/lib/octave/4.1.0+/../../../bin/liboctinterp-3.dll
/home/philip/devel/octdev/mxe/mxe_w64_20160308/tmp-default-octave/octave-4.1.0+/.build/libtool: line 10095: /home/philip/devel/octdev/mxe/mxe_w64_20160308/tmp-default-octave/octave-4.1.0+/.build/src/octave: No such file or directory
Makefile:27022: recipe for target 'doc/interpreter/doc-cache' failed
make[5]: *** [doc/interpreter/doc-cache] Error 127
make[5]: *** Waiting for unfinished jobs....

All binaries in usr/mingw..... were compiled so I just gambled and copied them over but once in Windows Octave didn't run.
Something to sort out tomorrow evening, it is past midnight here.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 15 Mar 2016 10:30:55 PM UTC, comment #85: 

Oops!  I'll really have to run make check before submitting each patch.  When I type "test textscan" in Octave, it just says that textscan is built in.  I think you mentioned that there is a longer incantation that will run test on built in functions, without a tedious "make check".

In both of those cases, I think the test should pass if "\n" is appended to the input string (which used to be the implicit default).

In the second case, I think that ignoring the missing field before the first delimiter is an explicit Matlab-compatibility workaround -- see line 3357 of textscan.cc

I'll look into the issue of builtin-defun-decls.h being used before it is created, although I know very little about the build process.

Lachlan Andrew <lachlan>In charge of this item.
Tue 15 Mar 2016 08:41:38 PM UTC, comment #84: 

With the latest patches, I get on Linux (Mageia 5):

>>>>> processing /home/philip/devel/octdev/oct410+VEyTS/libinterp/corefcn/textscan.cc-tst
***** test
a = textscan (",1,,4\nInf,  ,NaN", "", "delimiter", ",", "emptyvalue", -10);
assert (cell2mat (a), [-10, 1, -10, 4; Inf, -10, NaN, -10]);
!!!!! test failed
cat: dimension mismatch

where I think the test is wrong - there should be a trailing comma (delimiter) after the NaN in the test string.

***** test
str = "11, 12, 13,, 15\n21,, 23, 24, 25\n,, 33, 34, 35";
C = textscan (str, "%f %f %f %f", "delimiter", ",", "multipledelimsasone", 1, "endofline", "\n");
assert (C{1}', [11, 21, 33]);
assert (C{2}', [12, 23, 34]);
assert (C{3}', [13, 24, 35]);
assert (C{4}', [15, 25, NaN]);
!!!!! test failed
ASSERT errors for:  assert (C {4}',[15, 25, NaN])

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x2)       E(1x3)      Dimensions don't match

where textscan.cc apparently doesn't note the missing field before the double delimiter on the third line of str.
But Matlab r2016a prerelease doesn't note it either => Matlab bug?
In the other FAIL Matlab and Octave's textscan.cc do it right on the first line, but then 'multipledelimsasone┬┤ hasn't been specified. Experimenting a bit with Matlab shows that it is that parameter that screws it up.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 15 Mar 2016 07:13:05 PM UTC, comment #83: 

@Lachlan:
After copying my default Octave tree (clone of savannah repo) to dev_TS, applying your patches, bootstrap, goingto a separate build dir and doing configure ; make ... I get:

:
  GEN      libinterp/corefcn/utils.df
../dev_VETS/libinterp/corefcn/textscan.cc:1061:33: fatal error: builtin-defun-decls.h: No such file or directory
#include "builtin-defun-decls.h"
                                 ^
compilation terminated.

If I start with just a default repo clone and build that first, then apply your patches, building with textscan.cc succeeds.

File "builtin-defun-decls.h" seems to be generated too late? (AFAICS with my limited build system proficiency)

Philip Nienhuis <philipnienhuis>
Project Member
Tue 15 Mar 2016 07:01:49 PM UTC, comment #82: 

JWE,
Thanks for those hints.

In return:
If you have Windows & Linux on the same PC and if you can somehow access your Linux file system from the Windows side, it is even easier:
In windows you can just run octave-cli-4.1.0+.exe directly from the ./mxe-octave/dist/octave-<date>/bin subdir on the Linux file system.
From an MSYS shell, if you add & export the mxe-octave/dist/octave-<date>/bin subdir to the PATH, you can even run octave-gui.exe.
So one doesn't even need to install Octave on the Windows side.

I did this until a year ago but then it somehow didn't work anymore. But triggered by your hints I just checked and for some other unknown-to-me reason it works fine.

This is all by virtue of having a complete Octave installation tree "installed" in mxe-octave/dist/
In the past I've modded the mxe-octave Makefile to omit the date/time postfix in the dist/octave<date/time> subdir so I could have a batch file on the Windows side set up PATHs etc more easily.

It still involves rebooting (for a multiboot system like I have), I suppose that with virtualised Windows installations on the Linux host even more time can be saved.

Anyway it would be even easier if we could just build Octave on Windows natively again.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 15 Mar 2016 02:11:24 PM UTC, comment #81: 

For Windows, you can build an Octave distribution once using mxe-octave and install everything on the Windows system.  Then you can build just Octave with "KEEP_BUILD := 1" set in the mxe-octave Makefile.  that will leave the tmp-octave build directory intact.  Then you can set your PATH to include the ..mxe-octave/usr/bin directory (the one where the mxe-built cross tools are installed) and go to the Octave directory, make changes, and run make as you normally would.  If you are just making changes that affect a single shared library, you can just copy that single modified shared library over to the bin directory on the Windows system.  So it is not that time consuming to make incremental changes and test them.  Maybe we can make this even easier, but it has worked reasonably well for me in the past.

John W. Eaton <jwe>
Project Administrator
Tue 15 Mar 2016 10:56:21 AM UTC, comment #80: 

@Ben:
What is easiest appears to be a matter of taste :-)
I figured that having a binary textscan as separate .oct file would be easiest to debug as rebuilding Octave completely would obviously be more troublesome than just rebuilding a stand-alone .oct file. Especially for Window where for each tiny modification to an integrated textscan.cc, Octave:
- first has to be rebuilt in Linux ("make dist"),
- then cross built in mxe-octave, and then
- installed on the Windows side.
For testing in Windows, that is still valid.
But as Octave seems to build fine on OSX these days it may well be moot for that platform.
Anyway I think textscan.cc is now so good that I do not expect significant code changes anymore. But you never know....

@Lachlan:
Removing textscan.m is indeed a good idea now :-)
I hope to have time tonight for cross-building & testing.

In the mean time I'll continue work on overhauling strread.m and textread.m.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 15 Mar 2016 08:59:17 AM UTC, comment #79: 

Philip, thanks for checking the Matlab behaviour.

The latest patch (which still needs file #36334) handles the example

textscan ("10 a 20 b\n 30 c 40", "%d %s", 4)

correctly.

Since things are getting close to applying, this patch uses the name "textscan", removes "textscan.m", and updates the module.mk files accordingly.  jwe, it (plus file #36334) is ready for you to review.

(file #36643)

Lachlan Andrew <lachlan>In charge of this item.
Mon 14 Mar 2016 11:41:32 PM UTC, comment #78: 

@Philip,

I'd find further development to be easier once textscan.cc is applied to the sources. Are you suggesting that committing textscan.cc as it is would make the job more difficult?

Ben Abbott <bpabbott>
Project Member
Mon 14 Mar 2016 06:18:46 PM UTC, comment #77: 

@Ben, JWE:

I'd still like to run strread.m's test suite through textscan.cc, so that using it as backend for strread.m is easier.
In my previous comment #72 and comment #76 I showed that to that end a (hopefully) small mod to textscan.cc is required.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 14 Mar 2016 06:13:43 PM UTC, comment #76: 

Lachlan,

It's just that to use textscan.cc as backend for strread.m and textread.m, absence/presence of a trailing EOL matters.

Matlab (at least r2016a prerelease) gives:

>> textscan (['1 2' char(10) '3'], '%d %f')
ans =
    [2x1 int32]    [2]

>> textscan (['1 2' char(10) '3' char(10)], '%d %f')
ans =
    [2x1 int32]    [2x1 double]

>> textscan (['1 2 3' char(10)], '%d %f')
ans =
    [2x1 int32]    [2x1 double]

>> textscan (['1 2 3'], '%d %f')
ans =
    [2x1 int32]    [2]

Similarly when reading from file:

>> fid1 = fopen ('txtscn.dat')
fid1 =
     3

>> C = textscan (fid1, '%d %d')
C =
    [2x1 int32]    [2]
>> fclose (fid1);

>> fid2 = fopen ('txtscn2.dat')
fid2 =
     3
>> C = textscan (fid2, '%d %d')
C =
    [2x1 int32]    [2x1 int32]
>>

where txtscn.dat contains "1 2\n3" and txtscn2.dat "1 2\n3\n".

This behavior is the same as that of Matlab's strread:

>> [a, b] = strread ('1 2 3', '%d %d')
a =
     1
     3
b =
     2
>> [a, b] = strread (['1 2 3' char(10)], '%d %d')
a =
     1
     3
b =
     2
     0
%% note int32 has no NaN so here we get 0
>> [a, b] = strread (['1 2' char(10) '3'], '%d %d')
a =
     1
     3
b =
     2
>> [a, b] = strread (['1 2' char(10) '3' char(10)], '%d %d')
a =
     1
     3
b =
     2
     0>>

Philip Nienhuis <philipnienhuis>
Project Member
Mon 14 Mar 2016 01:28:10 AM UTC, comment #75: 

I've made a couple more changes (one was in handling expressions like %[a-z], and another to do with uneven columns).  I'll post the patch soon.

Philip, the issue you mention is just my interpretation of the Matlab docs: "If there are missing values and an end-of-line sequence at the end of the last line in a file, then textscan returns empty values for those fields. This ensures that individual cells in output cell array, C, are the same size."  It will actually simplify the code to make it not distinguish between files and strings, if that is what you would like.

Lachlan Andrew <lachlan>In charge of this item.
Sun 13 Mar 2016 11:59:55 PM UTC, comment #74: 

It's on my list to review and include in the Octave sources.  I'll try to do that this coming week.

John W. Eaton <jwe>
Project Administrator
Sun 13 Mar 2016 11:56:27 PM UTC, comment #73: 

Now looks to be a good time to commit the C++ version of textscan. Is there any reason not to do that?

What still needs to be done?

  • Have all the tests from textscan.m been included?
  • The changeset needs to be updated to remove the old textscan.m
  • ytextscan needs to be renamed to textscan.
  • Does the code meet our coding standards (my C++ is very bad, so I'm not able to say).

What else?

Ben Abbott <bpabbott>
Project Member
Sun 13 Mar 2016 09:57:26 PM UTC, comment #72: 

Hi Lachlan:
Never mind comment #71, ytextscan has no FAILs on Windows. Sorry for the noise.

I've been amending (weeding) strread to have (y)textscan() as a backend. To be consistent :-) I've provisionally renamed my dev strread to "ystrread.m" (attached). That is still a work in progress, as you'll understand.
The tests are still in place with "strread" replaced by "ystrread" to find out how far ytextscan can serve as backend for (y)strread.

I ran into the issue that ytextscan as it stands (files 36288 and 36566) still doesn't properly seem to process uneven column lengths:

>>  [a, b] = strread ("10 a 20 b\n 30 c 40", "%d %s", 4);
## Note strread.m !!!
>> b
b =
{
  [1,1] = a
  [2,1] = b
  [3,1] = c
}
>>  [a, b] = ystrread ("10 a 20 b\n 30 c 40", "%d %s", 4);
#### with ytextscan as backend
>> b
b =
{
  [1,1] = a
  [2,1] = b
  [3,1] = c
  [4,1] =
}
>> C = ytextscan ("10 a 20 b\n 30 c 40", "%d %s", 4);
>> C{2}
ans =
{
  [1,1] = a
  [2,1] = b
  [3,1] = c
  [4,1] =
}
>>

(Note the lack of trailing EOL in the text string - see comment 9. 10 and 46.)

(file #36626)

Philip Nienhuis <philipnienhuis>
Project Member
Thu 10 Mar 2016 04:44:56 PM UTC, comment #71: 

With file 36556 (+ 36288) I get 1 FAIL on Windows - but I suspect other issues (see bug #47367 & bug #47337).

Philip Nienhuis <philipnienhuis>
Project Member
Tue 08 Mar 2016 02:03:29 AM UTC, comment #70: 

Sorry for the delay responding. I've been working on build problems.

file #36562 works for me

>> test ytextscan
PASSES 76 out of 76 tests

Ben Abbott <bpabbott>
Project Member
Mon 07 Mar 2016 05:02:51 PM UTC, comment #69: 

Rebuilding with first a fresh clone of Octave-dev, only then applying the ytextscan patches & rebuilding works. So apparently some dependency chain isn't up-to-date.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 07 Mar 2016 04:16:58 PM UTC, comment #68: 

With latest tip (0c2ff0876a0f), after bootstrap/configure and applying #36288 and #36556, Octave doesn't build and complains about:

:
  GEN      libinterp/corefcn/ytextscan.df
../dev/libinterp/corefcn/ytextscan.cc:1062:33: fatal error: builtin-defun-decls.h: No such file or directory
#include "builtin-defun-decls.h"
                                 ^

Philip Nienhuis <philipnienhuis>
Project Member
Mon 07 Mar 2016 03:41:58 PM UTC, comment #67: 

The 2nd patch produced a seg-fault.

>> test ytextscan
panic: Segmentation fault: 11 -- stopping myself...
Segmentation fault: 11

I'll should verity that the seg-fault isn't due to another problem. I'll get to that later today.

Ben Abbott <bpabbott>
Project Member
Mon 07 Mar 2016 05:46:05 AM UTC, comment #66: 

Thanks, Ben.  It passed all but the last tests, and the attached patch fixes a bug with the test that it failed.  Could I impose on you to test this one too?  There's no need to test "textscan" -- this patch doesn't change that.  Just test "ytextscan".

(file #36562)

Lachlan Andrew <lachlan>In charge of this item.
Sun 06 Mar 2016 01:06:45 AM UTC, comment #65: 

I applied the patches on Mac OS X. I did not encounter any new problems. I ran the tests on textacan and ytextscan.

>> test textscan
***** test
assert (textscan ("1i", ""){1},  0+1i);
assert (cell2mat (textscan ("3, 2-4i, NaN\n -i, 1, 23.4+2.2i", "")), [3+0i, 2-4i, NaN+0i; 0-i,  1+0i, 23.4+2.2i]);
!!!!! test failed
ASSERT errors for:  assert (textscan ("1i", "") {1},0 + 1i)

  Location  |  Observed  |  Expected  |  Reason
     ()           O            E         real != complex
>> which textscan
'textscan' is a function from the file /Users/bpabbott/Development/mercurial/default/sources/scripts/io/textscan.m
>> which xtextscan
>> test xtextscan
????? xtextscan does not exist in path
>> test ytextscan
***** test
f = tempname ();
fid = fopen (f, "w+");
d = rand (1, 4);
fprintf (fid, "  %f %f /* comment */  %f  %f ", d);
fseek (fid, 0, "bof");
A = ytextscan (fid, "%f %f", "CommentStyle", {"/*", "*/"});
E = feof (fid);
fclose (fid);
unlink (f);
assert (A{1}, [d(1); d(3)], 1e-6);
assert (A{2}, [d(2); d(4)], 1e-6);
assert (E);
!!!!! test failed
ASSERT errors for:  assert (A {1},[d(1); d(3)],1e-6)

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x1)       E(2x1)      Dimensions don't match

Ben Abbott <bpabbott>
Project Member
Sat 05 Mar 2016 11:22:19 AM UTC, comment #64: 

After a few more tests and bug fixes, I think the code is now ready to be re-integrated into corefcn rather than as a module.  I hope this will make it easier for Hazel to build on OSX.

The attached patch assumes file #36288 has already been applied.

(file #36556)

Lachlan Andrew <lachlan>In charge of this item.
Fri 04 Mar 2016 10:55:27 AM UTC, comment #63: 

Here is a first cut of supporting ReturnOnError.  I think there are still issues with stopping in the right column, but it passes the 75 tests.

(file #36544, file #36545)

Lachlan Andrew <lachlan>In charge of this item.
Fri 26 Feb 2016 05:49:59 AM UTC, comment #62: 

It looks like Savannah converts a 0 at the start of a line to an enumeration.  That will be handy to know...

I was proposing 'continue' as the value of ReturnOnError that gives the current behaviour, because any non-zero numeric value will convert to a logical "true" -- I was proposing an input that is not currently valid under Matlab.

OK, I'll have a go at it.

Lachlan Andrew <lachlan>In charge of this item.
Thu 25 Feb 2016 03:58:19 PM UTC, comment #61: 

Matlab on-line help on ReturnOnError says <when textscan fails to read or convert> "If true, textscan terminates without an error and returns all fields read. If false, textscan terminates with an error and does not return an output cell array."
(Hmmm..., no mention of file pointer.)

textscan.m couldn't properly implement ReturnOnError as the operation mode of its workhorse strread.m is vastly different from that of textscan.cc. The latter ploughs linearly trough a file, the former (strread.m) tries to parse & convert a complete file into a rectangular cell array and then parses columnwise.
IOW, textscan.m's/strread.m's behavior (largely ignoring read errors, you assessed that correctly) shouldn't be used as a reference.
In fact, textscan.m's texinfo help says so:

(.....) As the string or file is read by columns rather than by rows, and
because textscan is fairly forgiving as regards read errors,
setting this option may have little or no actual effect.

AFAIU your suggestion (mangled by savannah's markup):
We'd have to set ReturnOnError to:

  1. or false, to get default behavior (error out and return nothing);

1 or true, to stop at an error and return data as far as textscan.cc got til that error;
2 (?), to insert something <?EmptyValue?> for the erroneous field in the output array and try to continue reading the rest of the file.
Well, I like that suggestion.

In case of (1) it would be nice to return the file ptr position.

But Matlab help isn't clear as to what its textscan really does: option 2 or option 3? Based on what MLr2012a does, I think it is option 2:

>> textscan ('  1   2  s', '%d', 'ReturnOnError', 0)
Error using textscan
Mismatch between file and format string.
Trouble reading integer from file (row 3u, field 1u) ==> s
>> textscan ('  1   2  s', '%d', 'ReturnOnError', 1)
ans =
    [2x1 int32]
>> textscan ('  1   s  3', '%d', 'ReturnOnError', 1)
ans =
    [1]

As to try catch- I was thinking of how to implement it, not so much of how textscan should work from a user perspective.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 25 Feb 2016 03:03:14 AM UTC, comment #60: 

In Matlab, ReturnOnError returns a valid value when it fails to match a literal in the format string, or when it finds a number that doesn't have the correct syntax.  I don't think it is intended to be a try/catch mechanism to capture, for example, I/O errors.  If it is 0, then an error is thrown on invalid input.

In Octave (in both textscan.m and textscan.cc, AFAIK) both of those above errors are silently skipped, and parsing continues.  That means there isn't really a condition under which ReturnOnError would do anything.

We could emulate Matlab if you think that is desirable.  However, I think a better solution would be to have three values for ReturnOnError:
 'continue' (default) Read as much as possible, as Octive does now
 1 Return a partial data structure when a parse error occurs

  1. Throw an error when a parse error occurs.

That gives Matlab-compatible behaviour if an explicit Matlab-compatible value of ReturnOnError is given, but Octave's current behaviour by default.  However, Matlab code that is trying to be as liberal as possible by specifying ReturnOnError is now less liberal than the default.

I'm influenced by the TCP/IP philosophy: "Be liberal in what you accept, and conservative in what you send".  However, error checking is useful.

ExpChar has been implemented in all of these patches.  Matlab used to have a "BufSize" option, which has disappeared.  Since ytextscan has its own buffer, I'm considering implementing that option.  What do you think?

Lachlan Andrew <lachlan>In charge of this item.
Wed 24 Feb 2016 06:19:10 PM UTC, comment #59: 

Of course the new version works, I never doubted that :-)

I think it is prudent enough to start amending (stripping) strread.m and textread.m now; they'll probably be pushed only after textscan.cc has been integrated in core Octave. I could attach the fixed versions here.

If "ReturnOnError, 1" is specified textscan should return the elements already read and return a file pointer pointing to a position immediately preceding the offending field in the file. As a C++ n00b I'd think of a try-catch but the new error functions may be better suited.

There's another not-yet-implemented property/value pair that Matlab does have: ExpChar.

I hope to find time soon to close a lot of related bug reports w.r.t. missing features in textscan/strread/textread and make them dependent on this report. There must be 5 or 6 of them (maybe even more), several assigned to me.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 23 Feb 2016 10:15:51 PM UTC, comment #58: 

Philip, I'm glad the new version works.

Would it be prudent to give textscan some testing as part of the default codebase before migrating strread and textread?

I'll take a look at the texinfo header.  At the moment, ReturnOnError has no effect, from memory.  The code always tries to keep going.  Should it be more strict with its error checking and try to emulate Matlab more closely?

Lachlan Andrew <lachlan>In charge of this item.
Mon 22 Feb 2016 09:33:40 PM UTC, comment #57: 

Thanks for the updated ytextsan.c, builds & runs fine (PASS 69 tests).

I'll start overhauling strread.m and textread.m to invoke textscan.cc as backend. I also noted some inconsistencies in the texinfo header, a.o. ReturnOnError still seems to be the old textscan.m text (that worked columnwise).

Philip Nienhuis <philipnienhuis>
Project Member
Sun 21 Feb 2016 08:50:13 PM UTC, comment #56: 

Sorry, Philip.  I hadn't realized that bzero was only in POSIX.  The latest ytextscan replaced is by memset, which I think is standard C.  It should now build with mkoctfile.

(file #36416)

Lachlan Andrew <lachlan>In charge of this item.
Sun 21 Feb 2016 02:14:50 PM UTC, comment #55: 

How to compile ytextscan.cc?

I had to compile (previous) ztextscan.cc separately as well (it didn't build automatically withe the rst of Octave)

On windows (Octave-4.1.0+):

>> mkoctfile ytextscan.cc
ytextscan.cc: In member function 'char* dstr::read(char*, int, char*&)':
ytextscan.cc:2500:45: error: 'bzero' was not declared in this scope
               bzero (eob, size - (eob - idx));
                                             ^
ytextscan.cc:2511:40: error: 'bzero' was not declared in this scope
                 bzero (buffer, size - i);
                                        ^
warning: mkoctfile: building exited with failure status

Philip Nienhuis <philipnienhuis>
Project Member
Fri 19 Feb 2016 10:18:23 PM UTC, comment #54: 

For the record, here are the latest files.  They incorporate a few fixes based on Hazel's feedback for MacOS.  We still haven't been able to run the tests on MacOS, so there may be a few more changes.

This new version also has internal buffering which gives a speed up factor of 3 on my workload.  It would be interesting to know what speed up (or slow down) it gives for other workloads. Having missing data seems to slow it down a lot; I haven't compared it to the slowdown of other versions for that case.

(file #36400, file #36401)

Lachlan Andrew <lachlan>In charge of this item.
Sun 14 Feb 2016 01:15:11 PM UTC, comment #53: 

@Lachlan:
Just to confirm, I tested this ztextscan.cc and it works fine. EOL presence/absence is picked appropriately => I think it can be invoked as backend for a replacement strread.m right away.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 14 Feb 2016 12:25:11 PM UTC, comment #52: 

Yes, TMW seem to have improved textscan in several ways in recent releases, and their documentation seem quite good.

Current the EOL at EOF thing in textscan.cc uses a seek to one position before the EOF.  Technically that is undefined behaviour (because, for example, the file could be in UTF-8 and the last byte may not be a whole character).  However, most of Octave breaks for non-ASCII files, so I think it should be OK.

FWIW, I'm attaching a patch to enable tellg/seekg that uses ftello and fseeko as jwe recommended.  I haven't yet tested that it works properly beyond 4GB; that will have to be tested on as wide a range of compilers/systems as possible, too.

The patch seems harmless; nothing currently relies on being able to seekg/tellg (because it is disabled) so there is nothing that can be broken by a buggy implementation of it.  Once jwe is happy to apply it, textscan can be a module and it should be much easier to test.

(file #36334)

Lachlan Andrew <lachlan>In charge of this item.
Sun 14 Feb 2016 11:49:33 AM UTC, comment #51: 

@Lachlan, as to trailing EOL before EOF, good catch. I haven't seen that documented before - though I may easily have overlooked it. I do note that TMW's textscan documentation became quite a bit more extensive since I last looked some years ago.

Anyway I don't think I'll bother to fix it for textscan.m :-)

If this is already working fine for textscan.cc it should work right away for strread.m; saves us a bit of hassle.

Philip Nienhuis <philipnienhuis>
Project Member
Sat 13 Feb 2016 10:41:59 PM UTC, comment #50: 

OK.  I think I've finally understood what jwe was suggesting.  It should be straightforward, like you both said.

Lachlan Andrew <lachlan>In charge of this item.
Sat 13 Feb 2016 10:39:02 PM UTC, comment #49: 

Philip, I think textscan also cares.  From http://au.mathworks.com/help/matlab/ref/textscan.html:

  If there are missing values and an end-of-line sequence
  at the end of the last line in a file, then textscan
  returns empty values for those fields.  This ensures that
  individual cells in output cell array, C, are the same size.

OK.  Once the seek issue is resolved, I'll write some unit tests.

Lachlan Andrew <lachlan>In charge of this item.
Sat 13 Feb 2016 10:24:29 AM UTC, comment #48: 

@Lachlan:
Just to be sure we're tuned on the same wavelength: it is strread, not textscan, that cares for is-the-last-char-an-EOL.
It gets important for textscan.cc only if we will use textscan.cc as backend for strread.

AFAIK textscan in Matlab happily returns data vectors/arrays that can have uneven length. To that end it only looks at the amount of data fields, not at the last characters of the file/string.

And yes, in case of Matlab that may become an issue with the CollectOutput param - I never tried though.

As to comment #46, if you're referring to Octave's textscan.m, you might have discovered a new bug. (I'll check later)

Philip Nienhuis <philipnienhuis>
Project Member
Sat 13 Feb 2016 04:52:01 AM UTC, comment #47: 

Philip, here is a version that seems to handle EOF of files the way that Matlab's does.

I personally think that they just had a bug in their code that they decided to "fix by documentation", and I prefer the version that always pads columns.  However, it's your call which to use.

If you definitely want to go with EOF-dependent behaviour, I'll test this more thoroughly and write some unit tests, once the tellg/seekg issue is resolved.

(file #36325)

Lachlan Andrew <lachlan>In charge of this item.
Sat 13 Feb 2016 03:55:09 AM UTC, comment #46: 

Philip, I've been looking at the uneven-lines issue.

Under Linux at least, the current textscan seems to output uneven lines even if the last character in the file is an end-of-line.  However, it outputs even lines if the last two characters are bot end-of-line.  My guess is that the last EOL gets swallowed by the process that reads in the line.  My guess is also that most applications don't care whether or not the output is padded when the last scan is incomplete; garbage-in-garbage-out...  Still, I'll keep trying to get it to work, for completeness.

octave:1> f = fopen ("partial.txt")
f =  3
octave:2> a = textscan (f, "%d %d %d")
a =
{
  [1,1] =

    1
    4

  [1,2] = 2
  [1,3] = 3
}

$ od -h --format x1 partial.txt
0000000  2031  2032  0a33  0a34
        31 20 32 20 33 0a 34 0a

Lachlan Andrew <lachlan>In charge of this item.
Fri 12 Feb 2016 11:23:29 AM UTC, comment #45: 

Philip, I was forgetting the EOL at EOF issue.  I'll look into that now.  It should be quite straightforward.  (Famous last words...)

I think that replacing tellg and seekg by tello and seeko would be a big headache.  Perhaps jwe can do that one instead.

Lachlan Andrew <lachlan>In charge of this item.
Fri 12 Feb 2016 09:46:14 AM UTC, comment #44: 

@JWE:
Yes any help is welcome, thanks.
Currently we have two things to sort out:

1. does it work well on other platforms than Linux and Windows? (e.g., OSX) ?

2. if textscan.cc is to be used as a backend for strread.m, we have to do something with absence/presence of a trailing EOL in text files / text strings, as strread.m uses that to decide if output arrays should be padded to equal length. Similarly for textread.m, I think.
Maybe textscan.cc needs an undocumented flag so that it knows it was called from strread.m and needs to adapt its behavior.
Once there, strread.m and textread.m have to be overhauled. I'd happily take that up. That can be done somewhat independent of textscan.cc

I adviced to make it a stand-alone module (temporarily) to allow easier debugging, esp. on other platforms like mxe/Windows.
But maybe that is moot as based on my own use, it works fine on Windows and on *nix-like platforms like OSX, Octave incl. textscan.cc can be built "natively" (unlike Windows).

@Lachlan:
If there is a seek()-like function as JWE said, theoretically it should be possible to map pointer(s) into a buffer into a file pointer. It implies a bit of bookkeeping, sure, esp. when multiple whitespace and multi-char line endings and delimiters come into play, but maybe not that daunting.

FTR, I once had a textscan.m working that did exactly that, using pointers handed to it by an extra output arg. from strread.m. It even worked when reading from text strings using an extra output arg, something that Matlab doesn't do.
At the time I didn't pursue further as inside strread.m it soon became a headache, due to the way strread.m is set up.
With textscan.cc it could be a lot easier as that linearly processes a file, unlike strread.m that works on a rectangular cell array from which a file pointer had to be inferred while all delimiters and whitespace already had gone after the original file text string was morphed into that cell array.
(Nowadays textscan.m only keeps pointers to lines in the text file.)

Philip Nienhuis <philipnienhuis>
Project Member
Fri 12 Feb 2016 05:45:14 AM UTC, comment #43: 

Philip, yes, there is seekg and tellg.

However, Octave explicitly disables them.  (jwe: thanks for your offer of help -- re-enabling these is the min help we need.)  They require functions that are commented out in c-file-ptr-stream.cc.  Take a look at patch  textscan_enable_seek.cset  file #36288.

I asked why these were commented out, and jwe said

On 10/26/2015 01:14 AM, LachlanA wrote:

    My biggest concern is that it needs tellg(), which was explicitly disabled
    (#if 0) in libinterp/corefcn/c-file-ptr-stream.cc with the cryptic comment
    "FIXME", giving no indication of why it was disabled.

Probably because of potential type problems of int vs off_t.  It looks like the c_file_ptr_buf::seek and c_file_ptr_buf::tell functions now use fseeko and ftello, so tellg could probably just be implemented using those.

I didn't really understand the concern, since the types are std::streamoff  std::streampos rather than int or off_t.  I also don't understand the internals of C++ streams well enough to be confident of re-implementing  tellg and seekg  in a way that works for both files and string streams.  The web says just to implement seekoff and seekpos in the streambuf subclass.

I've used textscan with files that cross the 4GB threshold between 32 and 64 bits (with octave_idx_type 32 bits) and didn't notice any problems, but I'd by happy to do some more testing on different configurations if someone suggests which.

You're right that reading ahead into a buffer is useful.  I was in the process of doing that when I came up with the problem that textscan is supposed to leave the file pointer pointing to the next item to read.  Once it has been read into the buffer, we're stuck unless we can seek back.

Lachlan Andrew <lachlan>In charge of this item.
Thu 11 Feb 2016 06:28:55 PM UTC, comment #42: 

seek and tell should work for Octave's stream classes that are based on FILE pointers, and that are working on files.  Seeking on a pipe won't work.

Maybe it's time to move this into the core?  I could help with that.  I think it belongs in the corefcn directory along with the other octave file I/O functions, but possibly in a separate file.

John W. Eaton <jwe>
Project Administrator
Thu 11 Feb 2016 05:14:39 PM UTC, comment #41: 

? is there no binary equivalent for fseek () ?

What many other modules like this do (e.g., csv2cell() in the io package) is to read text into a buffer and operate (read) on that.  I can't imagine that textscan() wouldn't do similar, but probably it is shielded from the programmer? (I really don't know, I'm just a c++ n00b and then esp. wrt Octave's C++ function universe.)

I think that -depending on the situation/format specifier at hand, it is often required to do some read-ahead ("peeking") at what follows next. Complex fields are one example, "cuddling" literals is another.

When relying solely on C/C++ functions like [f,s]scanf I think some flexibility is sacrificed for speed; which is probably the reason Matlab's textscan/strread often hickup on some text files that Octave's current strread happily accepts (with other limitations, granted).

Philip Nienhuis <philipnienhuis>
Project Member
Thu 11 Feb 2016 12:46:33 PM UTC, comment #40: 

It is impossible to have a completely stand-alone version -- at least without terrible inefficiency.

Consider a file containing the string "0.1234+0.1234i 0", and the sequence of statements

textscan (fd, "%f", 1);
textscan (fd, "%f", 1);

The first reads only a single number (the complex one) and the second reads 0.

Now consider that the "i" is absent from the file.  The first read should return 0.1234 and the second should also return 0.1234.  To do that,  textscan  must leave the file's read position at the "+" sign, but it had to read ahead until the "i" (or subsequent space) to decide whether or not to stop there.

Unless we can seek backwards (which Octave currently disallows), the only way we can implement that is to count how many characters we have read, close the file, and re-read the appropriate number of characters.

I use textscan on a 60GB file.  Re-reading the entire file after each call is not an option...

For the moment, let's wait for our kind OSX testers to see how this works.  Let me know what you'd like me to do w.r.t. the dbstop issues.  I'll try to give MXE a go over the weekend.

Lachlan Andrew <lachlan>In charge of this item.
Wed 10 Feb 2016 06:12:32 PM UTC, comment #39: 

I separated out only the two comment tests (C/C++ style) because during compilation gcc would complain about those comment characters.
All tests always pass in my experimental builds so I don't follow what would be wrong.

My suggestion to make it a separate module was perhaps primarily motivated for the experimental test phase.  Once we're confident it runs fine on all platforms it could be glued back (into data.cc?) but maybe it's better to leave it as an .oct file to keep maintainability easier.

If you have a solution to make it completely stand-alone that would be a good temporary kludge :-)

When I have time I'd rather like to work on the dbstop/breakpoint issues.

Philip Nienhuis <philipnienhuis>
Project Member
Wed 10 Feb 2016 09:44:35 AM UTC, comment #38: 

I don't think it is possible to make textscan as a module with the current Octave core, because Octave explicitly disables seekg.  However, with the small patch in textscan_enable_seek.cset, it is possible to make a module -- in textscan_module.cset.

It places the files in libinterp/corefcn because that is where it should probably eventually live, but it doesn't provide the glue to build it.  It needs mkoctfile.  This still has the test in the .cc file.  There was a typo in one of the tests, which stopped it from compiling; was the reason you said the tests had to be separated?  (Sorry for being so careless ):

(FWIW, Last time, when I tried to text the modules, it was running the original .m script...)

(file #36288, file #36289)

Lachlan Andrew <lachlan>In charge of this item.
Thu 04 Feb 2016 08:36:31 AM UTC, comment #37: 

Ignore the files I just posted.  They seem to be missing several function; I'm not sure how they seemed to work last time!

Lachlan Andrew <lachlan>In charge of this item.
Thu 04 Feb 2016 01:39:33 AM UTC, comment #36: 

Thanks for the update, Philip.  Of course I don't mind Ben trying the patch.

I've just attached  xtextscan.{cc,h}  which compile using mkoctfile.  They pass all tests, but are six times slower than the compiled-in version -- probably a result of the extra layer of buffering I needed to emulate tell/seek without modifying the core.  I plan to profile them and find out what is going on (but it will take time to remove all Octave code, so gprof works -- I haven't managed to get oprofile to give plausible profiles).

(file #36245, file #36246)

Lachlan Andrew <lachlan>In charge of this item.
Wed 03 Feb 2016 08:58:35 PM UTC, comment #35: 

FTR, here I just upload a cset, based on your latest patch, that still applies almost cleanly to the dev tree on Feb 3, 2016.

I've moved the C/C++ comments tests in it to test/xtextscan.tst

@Lachlan:
Ben Abbott might be interested in trying out xtextscan(). He's on Mac OSX as well. I'll ping him, hope you don't mind.
@Ben: an experimental binary textscan, for the moment disguised as xtextscan...  (but AFAICS it works very well)

(file #36243)

Philip Nienhuis <philipnienhuis>
Project Member
Tue 19 Jan 2016 10:42:43 PM UTC, comment #34: 

Philip,

Thanks for the ping.  I haven't forgotten this either.  I was assuming the ball is in my court to produce a stand-alone version that doesn't require changes other internals.  The main decision there is how much to slow down the typical case to allow things like arbitrarily long strings, which the current implementation handles.

I got an offer from Hazel Jenkins to test it on OSX, but my reply (to hjenkins@uvic.ca) bounced.  I was going to try again to reply once I have something that doesn't require her to recompile the whole thing.

My goal with the bugs is to keep the number of open bugs below 1000, which it is rapidly approaching.  It has been fun getting to know Octave better (and you and Torsten and Rik), but I should spend more time on my job one of these days...

Lachlan Andrew <lachlan>In charge of this item.
Tue 19 Jan 2016 09:23:56 PM UTC, comment #33: 

Lachlan, just to let you know that I didn't abandon this patch.

The comment tests can't reside in the .cc file, perhaps they should move into the tests directory where you put them initially.

I still want to try with the textscan.m/strread.m test suite to see what's missing (IIRC you mentioned the "cuddling literals").
Another thing is thinking about how to replace strread.m as backend by textscan.cc - BTW that was the idea of Ben Abbott.

In the mean time I got quite a few other assignments (privately and work-related), and I see you dived deep in Octave internals and old rusty bugs - good!

Philip Nienhuis <philipnienhuis>
Project Member
Wed 23 Dec 2015 06:42:37 AM UTC, comment #32: 

As to buffering (that one I overlooked) there are possibly examples in the Octave sources. Maybe dlmread.
Otherwise it doesn't look hard - I once had it running for other SW I wrote in C (long ago). The interesting parts come when there is a field at the end that's maybe crossing the buffer "border" - the "maybe" is the intruiging one :-)

Philip Nienhuis <philipnienhuis>
Project Member
Tue 22 Dec 2015 11:06:50 PM UTC, comment #31: 

That's a good suggestion.

However, I've been thinking about getting it to do its own buffering just for the (possible) performance improvement.  I'll make it a low priority task...

Thanks again for all your help in this.  I really value your experience.

Lachlan Andrew <lachlan>In charge of this item.
Tue 22 Dec 2015 10:58:17 PM UTC, comment #30: 

Lachlan,

Before venturing in too much work, better ask first in e.g., IRC or in the maintainers ML about more experienced core devs opinions.

After all, my idea was to just make debugging and re-building textscan() easier on non-Linux platforms.
I wouldn't want to interfere with the general embedding of core octave functions.
(Note that my C++ skills are quite lacking - I just figure it was a mere matter of adding #includes 8-O )

Philip Nienhuis <philipnienhuis>
Project Member
Tue 22 Dec 2015 10:27:03 PM UTC, comment #29: 

Philip,

I'm glad the new textscan seems to be working :)

To make a stand-alone version will take a fair bit of rewriting.  It currently relies on seek/tell, which don't work without patching the core.  I'll put on my to-do list to rewrite it to do its own buffering.  Ideally that should make it faster too, since getc() is currently a major consumer of time.

Merry Christmas,
Lachlan

Lachlan Andrew <lachlan>In charge of this item.
Sun 20 Dec 2015 10:04:15 AM UTC, comment #28: 

Lachlan,

Thanks for the new patch.
As I turned to other work-priorities this week (reading GIS raster files) I had only some chance to test textscan.cc, but I can tell that it really helped me a lot compared to the old textscan.m! (more robust / easier pattern on some special .csv files and much faster)

What I still want to do is try to make a stand-alone .oct from it (i.e., not depending on data.cc - if at all possible) so that it can be compiled (mkoctfile'd) stand-alone on Windows and Mac systems as well.

I'll see what I can do in the nextweek and ensuing Xmas holiday (depending on distractions :-) )

Philip Nienhuis <philipnienhuis>
Project Member
Sun 20 Dec 2015 05:06:15 AM UTC, comment #27: 

Greetings Philip,

I though I had replied to your most recent post, but it seems that it didn't go through.  Anyway, here is a patch that makes the changes you asked for.  (I had always planned to move the tests into the .cc file, but didn't want to have to recompile every time I tweaked a test.)

Let me know if you want anything else from me, such as fixing it to make a standalone .oct file.

Thanks for all your help and patience in this, and Merry Christmas.

(file #35803)

Lachlan Andrew <lachlan>In charge of this item.
Mon 14 Dec 2015 09:33:41 PM UTC, comment #26: 

For your peace of mind: on Windows I now get:

:
libinterp\dldfcn\qr.cc-tst .................................. PASS     48/48
  libinterp\dldfcn\xtextscan.tst .............................. PASS     69/69
  libinterp\octave-value\ov-base.cc-tst ....................... PASS      1/1
:

The patch still needs some polishing:

  • a rule in libinterp/module.mk to include libinterp/dldfcn/(x)textscan.h under the LIBINTERP_BUILT_DISTFILES header;
  • the tests can be included in (x)textscan.cc (at the end as a comment, see for example libinterp/corefcn/dlmread.cc), rather than be a stand-alone file.

In the next days I'll try to make a stand-alone .oct out of it, and I'll try it at work (usually a sort of stress test :-) ).

Thanks!

Philip Nienhuis <philipnienhuis>
Project Member
Mon 14 Dec 2015 06:31:54 AM UTC, comment #25: 

Oops!  Here it is this it (I hope).

(file #35739)

Lachlan Andrew <lachlan>In charge of this item.
Sat 12 Dec 2015 01:19:05 PM UTC, comment #24: 

BTW I think you you forgot to upload the new patch....

Philip Nienhuis <philipnienhuis>
Project Member
Sat 12 Dec 2015 12:09:52 PM UTC, comment #23: 

Thanks Lachlan, I'll try later this weekend.

Yeah the "what if it doesn't work as intended" issues are usually the most frustrating and nastiest and at the same time the hardest to anticipate. For me, mxe-octave is a routinely "It Just Always Works" so it's hard for me to imagine where it can go wrong.  And if it does (usually little things) I fix it in passing w/o much thought.
So, please amend the wiki if/where you can.

I doubt if I have time for the code sprint even while I live in a more favorable location (Netherlands); I have private matters (like helping my daughter with a -for her- particularly nasty part of her math homework). Maybe I can attend just half an hour or so.

Philip Nienhuis <philipnienhuis>
Project Member
Sat 12 Dec 2015 04:00:19 AM UTC, comment #22: 

Philip,

Thanks for creating the build.  I decided to figure out how I can build my own, and that kept me too busy to notice your URL and tips re mkoctfile.  Your additions to the wiki on mxe-octave are really clear.  However, there still seems to be some black magic involved, but it eventually built.  (If I get around to it, I'll update the wiki with a "what to do if things go wrong" section...)

Here is a new patch, that should pass all of the tests under Windows.  The problem was a bad "if" condition that caused a reference to an uninitialized look-up table for delimiters.  I have no idea why it worked on Linux.  Fortunately, it didn't need multiple cross-compiles.

The code I actually tested was my main code in which the function is called "textscan".  I did a global search-and-replace to xtextscan after taking the diff, so let me know if it doesn't apply cleanly.

I don't have access to MacOS to test it.  Sourceforge used to have a shared build farm, but I take it that Savannah doesn't.  If you can test it, please let me know how it goes.

After that, I suppose the next step is supporting textread etc.  Should we do them in C++ too, or just as scripts?  I think they have some conversion specifiers that textscan doesn't.  I'm inclined to make textscan accept more inputs than Matlab's so that all other functions are simple special cases of it.  Let me know what you think.

Have fun in the "sprint" if you take part.  I'm in Australia and so will be asleep when most of you are busy.

Lachlan Andrew <lachlan>In charge of this item.
Tue 08 Dec 2015 08:51:02 PM UTC, comment #21: 

I gave it a try, separately mkoctfile-ing textscan.cc / textscan.h.
But it errorred out due to several include files not being referenced (or not in the right order?).

To be able to build textscan.oct using mkoctfile is something that can be prepared on the Linux side. Just put textscan.cc/textscan.h in some subdir, navigate there from within Octave an give it a go.

Once this works on Linux I'm sure it'll work on Windows as well.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 08 Dec 2015 06:26:17 PM UTC, comment #20: 

BTW Lachlan,

This Windows debugging issue is exactly the reason I suggested making textscan() a standalone function to be built as an .oct file.
Then, in Octave on Windows or OSX or Linux or whatever, one can simply do:

mkoctfile textscan.cc

w/o having to make a new dist + cross-build it in mxe-octave + transfer it to + install it in Windows; only to catch the next bug for which a complete new cycle is required.

Hmmm .... now that I mention, I haven't even tried to see if (x)textscan.cc can be built as suggested as separate .oct file.....

(BTW I found that with the patch, on Linux, make check now gives one FAIL in data.cc. I'll have to investigate it further).

Philip Nienhuis <philipnienhuis>
Project Member
Sun 06 Dec 2015 11:44:35 PM UTC, comment #19: 

@Lachlan:
OK I've put up a debug version here on some cloud storage of mine that I had forgotten about:
https://prn183.stackstorage.com/index.php/s/W1W8vfqgLyyJZMz

Notes:
- I've renamed all instances of textscan in your patch with xtextscan
- textscan.m has been kept as-is;
- I've applied your dbstop mod from patch #8784. Note that that patch gives a failed hunk in ...error.cc (easily fixed). No idea if this patch will work in Windows :-)
- It also contains a Variable Editor patch (see patch # 8656).
- No OF packages have been included.
- Even then, unzipped contents amount to 2.7 GB  (hic). The zip to download is about 875 MB.
- You need to make Windows shortcuts yourself (to octave-cli.exe/octave.vbs --no-gui, octave-gui.exe/octavevbs --force-gui and the msys shell/cmdshell.bat

Philip Nienhuis <philipnienhuis>
Project Member
Sun 06 Dec 2015 03:11:56 PM UTC, comment #18: 

@Lachlan:
A debug build of Octave + textscan + dbstop patch is now finished, but it is huge: 903 MB (all dependencies + Octave are unstripped). Uploading to dropbox or any other web storage takes /*ages*/; apparently dropbox assigns low priority to such big uploads; moreover I wonder if it is worthwhile to have my own bandwidth used to sync it on all my devices where I won't use it (smartphone, tablet, etc).

So, if you want I could send it by wetransfer or some other web transfer service at your discretion; just email me privately.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 06 Dec 2015 03:01:07 PM UTC, comment #17: 

I've added a little more elaborate how-to on making Octave-for_Windows binaries on the wiki, see
http://wiki.octave.org/Windows_Installer

Philip Nienhuis <philipnienhuis>
Project Member
Sun 06 Dec 2015 12:14:39 PM UTC, comment #16: 

Yeah it is a bit involved :-)
See https://savannah.gnu.org/patch/index.php?8656 comment #40, it'll point to:
http://octave.1599824.n4.nabble.com/Contributing-to-Windows-MXE-and-Linux-versions-cannot-find-the-sources-on-MXE-td4671082.html
where I've outlined how I do my mxe builds. Added note: I configured mxe-octave with --enable-octave=default as that .mk file has the best info.
Note that I do not use the chksum stuff, in src/default-octave.mk I've replaced the checksum with "## No chksum" (w/o quotes).

I'm just building an mxe-octave (f w64) debug version right now, incl. your dbstop patch.  Hopefully it is in my dropbox within 2 hours (uploading takes the longer part).

Philip Nienhuis <philipnienhuis>
Project Member
Sun 06 Dec 2015 06:17:25 AM UTC, comment #15: 

Thanks for the tips about configure options.  I can't even figure out how to get it to compile my code -- by default it seem to grab the 4.0.0 code from the web and compile that.  The page at http://wiki.octave.org/Windows_Installer doesn't mention specifying the source.  Could you point me in the right direction?

I don't use debuggers much either; I usually just use lots of printf and re-compile iterations. Perhaps that will change since cross-compiling is so much slower.

Thanks again,
Lachlan

Lachlan Andrew <lachlan>In charge of this item.
Sat 05 Dec 2015 12:17:27 PM UTC, comment #14: 

Well OTOH it is true that 45 of the 62-or-so tests SUCCEEDED :-)
Chances are the FAILs are due to specific MinGW idiosyncrasies and perhaps some includes and stuff needed for MinGW that you (and I) weren't aware of.
(And that's only MinGW then; Mac OSX comes next.....)

I'll try to make a debug (unstripped) mxe-octave version with (x)textscan.cc this weekend; I can put it up on my Dropbox. It'll have to be a zipped version ("make zipdist"), as a complete nsis-installer including OF packages is much too big (> 2 GB seems to be problematic for MXE).

If you want to make an mxe-octave version yourself,  be sure to specify the relevant configure options (see ./configure --help), i.e. development tools, MSYS shell, gdb, do not strip binaries.
Once in Windows and doing "gdb octave-cli", don't be put off initially by the many many C/CPP error messages about wrong arguments and IIRC even a segfault, after that octave-cli will start fine nonetheless.

As regards gdb I'm a novice so I'm of little help. I know debuggers only from DOS assembly and Fortran program development (sic) long ago, but I assume the principles are largely similar.

Philip Nienhuis <philipnienhuis>
Project Member
Sat 05 Dec 2015 02:17:38 AM UTC, comment #13: 

Thanks for the tests.  This is rather disappointing (*sigh*).  Still, I find that sometimes OS incompatibility reveals other hidden bugs, so I hope this will improve the Linux functionality too.

Thanks also for the tip about mxe-octave.  I've just spent the morning doing things like trying to get msys configured under Wine -- I should have read your post carefully first!

The next update may take a while to come...

Anonymous
Sun 29 Nov 2015 06:08:04 PM UTC, comment #12: 

OK here are tests on MingW / Windows 7 64bit (cross-built w. mxe-octave for Win64).

There are 17 FAILs when running _run_test_suite_.m; one of those FAILs looks fishy as I think assert.m itself got it wrong (assert.m gives 1 FAIL running the test suite on MinGW)

See attached fntests.log (in which I've added the results of xtextscan() to compare).

(file #35593)

Philip Nienhuis <philipnienhuis>
Project Member
Sun 29 Nov 2015 02:30:06 PM UTC, comment #11: 

As to the textscan.m <=> textscan.cc confusion:
in your latest patch I renamed all occurrences of "textscan"with "xtextscan", dropped the hunk that removes textscan.m and added one that adds xtextscan.h to libinterp/module.mk (as that one is tracked by mercurial); that allows me to build a dist archive to cross-build a Windows binary installer in mxe-octave.
So the experimental binary textscan is now invoked as xtextscan().

Amended patch attached.

The examples on the Matlab textscan pages seem to work quite well under Linux. Testing on Windows comes later (some private assignments come first :-)).

As textscan.cc seems to work so well already I suppose it is time to initiate thinking about how to replace strread.m's innards by textscan.cc; same for textread.m.

(file #35589)

Philip Nienhuis <philipnienhuis>
Project Member
Fri 27 Nov 2015 12:47:59 PM UTC, comment #10: 

Yeah I figured the out-of-sync was due to something simple :-)

%[a-f] is also appealing to me, but I suppose (no time to check now) that Matlab will treat it as characters "a", "-" and "f" rather than a,b, c, d, e and f. Is there a monkey-proof way of allowing a-e syntax?

textscan.m doesn't implement returnonerror like ML but that is because it works columnwise rather than ploughing linearly through a data file. This can be fixed later in textscan.cc

"\n" on incomplete last lines - we'll have to look up what Matlab's textscan does.
One of the ambitions was to have a binary textscan serve as workhorse for strread.m and textread.m, rather than the other way round as it is now. Those functions have special treatment for "\n" on incomplete last "lines".
Also to be fixed later.

As to comments, I think we'd need to follow what Matlab does, optionally and where reasonable extended. Maybe also s/th to fix later...

I'll have a go at the new patch this weekend.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 26 Nov 2015 02:57:49 AM UTC, comment #9: 

Philip,

Thanks for the encouragement.  I've been programming C++ for decades, but I'm new at Octave libraries...

The reason the code was getting out of sync is it treated "-inf" as "-i" followed by "nf".  That works now, and 'inf' and 'nan' are also now handled by the code that reads numbers with non-standard expChars or field widths.

Other bug fixes are:
- It was manually clipping values of integers to the required range, and  assumed octave_uint8(-1) would be 255.  Since the constructors do the clipping already, that is removed.
- It didn't handles escapes in single quoted strings, like '%[^\n]'
- It replaces the library function pow10 by an explicit "pown"

The patch doesn't resolve the issue of co-existing with textscan.m, but it is still quite a way from production, so I hope you can live with that.

Some remaining matters of incompatibility with Matlab that need to be sorted are:

1. It treats, for example, %[a-f] as shorthand for %[abcdef].  That is slightly incompatible -- should we keep it?  The other slight disadvantage is that it calls code that was "static" in oct-stream.cc to parse the character set, and so requires edits to oct-stream.{cc,h} to make that function global.  We could duplicate the code, but that seems wasteful (and would be bad for maintenance, such as converting to unicode).

2. It doesn't stop on invalid numbers, or mismatched literals.  I textscan.m also doesn't, so I assume that is OK

3. In files with an incomplete last line, it doesn't distinguish whether or not the last line finished with \n.  My guess is that they did that just because it was easier in their code.  Should we add that test?  My preference is to keep the code cleaner by not doing so.

The other notable incompatibility is with textscan.m: my code only accepts literal commentStyle specifiers, like "//", not names like 'C', 'C++', 'shell' etc.  That is partly deliberate, because I can't see how to specify that "C" introduces a comment, as it does in Fortran source (though it is not clear that textscan is suitable for reading Fortran...).  Would you like me to put the short-cuts in?

Thanks,
Lachlan (whose surname is Andrew, just to be confusing :)

(file #35557)

Lachlan Andrew <lachlan>In charge of this item.
Sat 21 Nov 2015 09:57:23 PM UTC, comment #8: 

@Lachlan (or is it Andrew?):
Just this: I think creating a binary textscan isn't quite bad for a newbie ;-)

Philip Nienhuis <philipnienhuis>
Project Member
Sat 21 Nov 2015 04:42:06 AM UTC, comment #7: 

Thanks for the feedback.  I had been looking for some source of new tests, and it didn't occur to me just to look at the Matlab examples -- that shows what a newbie I am!

I had already removed  pow10  by the time I read this.  My motivation was that 5e-1 wasn't exactly 0.5.  It now calculates pow10 using integer arithmetic (no libraries).

I agree that it would be good to give the user a choice between the two implementations for a while

I've never used mkoctfile, but will give that a go, and get back to you in due course.

Lachlan Andrew <lachlan>In charge of this item.
Sun 15 Nov 2015 07:55:11 PM UTC, comment #6: 

Thanks for the new patch.

I was able to build Octave-dev with the patch and also cross-build it with mxe-octave, but it needed a bit of attention:

1. To make a dist archive ("make all dist") amenable for cross-building with mxe-octave, the file textscan.h must be included in the dist archive; all I know is that it doesn't happen automatically and it must be referenced somewhere. I just added it in libinterp/module.mk under LIBINTERP_BUILT_DISTFILES but that may well be the wrong place (although it worked).

2. textscan.cc invokes pow10() (from glibc I guess) but several other target hosts do not implement pow10, notably mingw. I tried to include it using gnulib but AFAIU that only supplies pow and powf. (I'm unfamiliar with gnulib, I just tried along the ways shown by other gnulib additions in the core Octave repo log.)
For the moment I used a kludge: simple replace pow10 by "10^"

3. I'd rather not drop textscan.m yet from the dev version until textscan() works sufficiently reliably. My suggestion is to provisionally either:
- rename textscan() to xtextscan, or
- rename textscan.m to otextscan.m (and replace all occurrences in that file).

Right, with that out of the way some tests.

- textscan() passes all 62 tests on Linux. On Mingw it cannot find the tests, I need to do some more work there :-)

The examples from the TMW site (http://nl.mathworks.com/help/matlab/ref/textscan.html) do not go that well.
The first two work fine, but all examples starting with reading "scan1.dat" give different results than Matlab.
On Linux:

>> fid = fopen ("scan1.dat")
fid =  19
>> C = textscan(fid, '%s %s %f32 %d8 %u %f %f %s %f')
C =
{
  [1,1] =
  {
    [1,1] = 09/12/2005
    [2,1] = 10/12/2005
    [3,1] = No
    [4,1] = Level3
  }
  [1,2] =
  {
    [1,1] = Level1
    [2,1] = Level2
    [3,1] = 2.2-.5i
    [4,1] = 34.90
  }
  [1,3] =

     12.340
     23.540
     11.000
     12.000

  [1,4] =

     45
     60
      0
    127

  [1,5] =

     0
     0
     0
    10

  [1,6] =

     Inf +   0i
       0 -   1i
     NaN +   0i
     100 +   0i

  [1,7] =

     NaN
     NaN
     NaN
     NaN

  [1,8] =
  {
    [1,1] = Yes
    [2,1] = nf
    [3,1] = /12/2005
    [4,1] = No
  }
  [1,9] =

     5.10000 + 3.00000i
     0.00100 + 0.00000i
         NaN + 0.00000i
     3.10000 + 0.10000i

}
>>

...and on Mingw (Windows) I got different results, yet again different with CRLF EOLs rather than with LF EOLs.

Supposedly textscan() gets out of sync somewhere. Based on my experience with strread.m/textscan.m I expect this to be a simple issue, as usual ;-)

It would be nice if textscan() can pass all examples on that Mathworks site.

In spite of all the above comments, I am very positive about this C++ textscan version!

Philip Nienhuis <philipnienhuis>
Project Member
Sat 14 Nov 2015 12:50:08 PM UTC, comment #5: 

<just short comments, little time now>

ML's textscan (and for that matter, strread and textread) probably suffer from what in my native mother tongue is called "the Xmas-tree effect" - too many bells and whistles added w/o much of an underlying structured philosophy; and sometimes the options can bite each other.

Like you I'm in favor of offering better functionality than ML. For me the current textscan.m+strread.m combo often works better than ML's stuff, it can do things that ML can't, like "cuddling" literals.

I deliberately tried the patch against 4.0.x rather than 4.0.0, in the hope it wouldn't yield too many failed hunks, as 4.0.0 is IMO already outdated.

AFAIK an independent .oct file can be made fairly easily as long as the mkoctfile command is fed all required include file locations (i.e., probably also some needed for oct-stream.cc) and maybe other dependencies outside of those for Octave itself.
Again, that's only a suggested temporarily convenience to avoid having to rebuild Octave to build & debug just one function (that hasn't yet formally been included in core either). It means one can develop textscan() in some subdir completely separate from Octave by just running "mkoctfile <options>". No .mk files should be edited, core Octave's build tree shouldn't be affected in any way. But that's all your pick :-)

Philip Nienhuis <philipnienhuis>
Project Member
Sat 14 Nov 2015 01:56:02 AM UTC, comment #4: 

Thanks for the new tests.  I've attached a patch (relative to the current "default" branch this time).  It fixes several bugs that the tests highlighted.

I had mentioned that I have changed some of the tests in textscan.m; I've also changed some of the new tests you have supplied.  A diff of the changes (other than those due to different error messages) is attached as "tests_diff.txt", with some comments about why I changed the tests.  The complete set of tests I have used is in  textscan.tst.

Matlab's textscan seems quite buggy, and also some of its decisions seem quite arbitrary -- such as the fact that a %x.yf conversion will ignore the "e+zz" exponent if  y  is too small.  It also doesn't support contractions such as %[a-z], but jwe's code did.  I'm in favour of supporting them, even though it reduces compatibility slightly.

FWIW, the reason the patch didn't apply cleanly to the current hg source is that it is relative to 4.0.0.  That is mentioned in the comment associated with the file; sorry for not making it clearer.  The reason for the convoluted manner of including it from oct-stream.cc was that I didn't have autotools, and didn't have enough internet access to install them.  Without them, I couldn't tell "make" to include a new file in the build (editing Makefile by hand could have been overwritten by ../configure, and even editing Makefile.in risked being overwritten if hg had somehow decided Makefile.am was older than Makefile.in), and so had to force it to be compiled some other way.  I have now installed them, and subsequent patches will be relative to the default development branch.

The current patch creates an .oct file as you suggest.  Are there instructions on how to do that properly?  I have edited module-files to include a new #include path, but I'm not sure if that is the right way.  If there are no instructions, I'll add it to my newbies' FAQ :)

Thanks!

(file #35452, file #35453, file #35454)

Lachlan Andrew <lachlan>In charge of this item.
Tue 03 Nov 2015 04:38:28 PM UTC, comment #3: 

As to comment #1, I discovered the bit width stuff, sorry.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 01 Nov 2015 06:11:27 PM UTC, comment #2: 

In addition here are some tests of my strread.m implementation of %[] and %^[], rewritten for textscan.
They work with the m-file implementations of textscan.m and (patched, see patch #7817) strread.m 
(I'll upload the latest patches + complete patched strread.m there later tonight, not to be applied but just for reference)

(file #35357)

Philip Nienhuis <philipnienhuis>
Project Member
Sun 01 Nov 2015 12:06:43 PM UTC, comment #1: 

As mentioned in the maintainers ML thread, here's a first set of tests currently implemented in strread.m that are actually meant for textscan(), in the form of a txt file.
An additional test block is for bit width specifiers.

Perusing the list of strread.m tests I think that maybe 3/4 of all those tests should be amenable as tests for textscan() as well, after a little rewriting for different output arg format.

I tried to build textscan by "hg import"-ing it in the stable branch (upcoming 4.0.1) but it didn't apply cleanly.
Which makes me wonder why it is made part of oct-stream.cc at all, rather than being a separate .oct file.
If it were in a separate .oct file it would be easier to maintain it; i.e., a complete rebuild + maybe installation of Octave wouldn't be required then, just a recompile using mkoctfile in an existing Octave. If required it can also be absorbed back.

I think textscan() as it stands probably still needs quite a bit of attention once it works. Things to implement and test are bit width specifiers, remembering input file position (for text string input as well) and probably quite a bit of ironing out bugs and little issues.

(file #35356)

Philip Nienhuis <philipnienhuis>
Project Member
Mon 26 Oct 2015 05:09:50 AM UTC, original submission:  

This patch replaces  textscan.m  by a C++ implementation, which is both faster and more complete.  For example, it implements %[...] and %[^...] and %q.

The patch is not yet ready to apply directly; because I don't have all build tools, I have not edited Makefile.am  and so textscan.cc is included in oct-stream.cc instead of being its own make target.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36694:  textscan-return_on_error-updated.diff added by bpabbott (2KiB - application/octet-stream)
file #36693:  textscan-return_on_error-updated.diff added by bpabbott (830B - application/octet-stream)
file #36677:  textscan-valgrind-log.txt added by jwe (52KiB - text/plain)
file #36626:  ystrread.m added by philipnienhuis (13KiB - text/x-objcsrc)
file #36544:  ytextscan.cc added by lachlan (101KiB - text/x-c++src)
file #36545:  ytextscan.h added by lachlan (14KiB - text/x-chdr)
file #36416:  ytextscan.cc added by lachlan (97KiB - text/x-c++src)
file #36400:  ytextscan.cc added by lachlan (97KiB - text/x-c++src)
file #36401:  ytextscan.h added by lachlan (14KiB - text/x-chdr)
file #36334:  textscan_enable_seek_v2.cset added by lachlan (2KiB - application/octet-stream)
file #36325:  textscan_EOF_handling.cset added by lachlan (107KiB - application/octet-stream)
file #36288:  textscan_enable_seek.cset added by lachlan (1KiB - application/octet-stream)
file #36289:  textscan_module.cset added by lachlan (107KiB - application/octet-stream)
file #36245:  xtextscan.cc added by lachlan (94KiB - text/x-c++src)
file #36246:  xtextscan.h added by lachlan (13KiB - text/x-chdr)
file #36243:  xtextscan-9jan2016.cs added by philipnienhuis (107KiB - text/x-csharp)
file #35593:  fntests.log added by philipnienhuis (15KiB - text/x-log)
file #35557:  textscan_diff_default_20151126.txt added by lachlan (106KiB - text/plain - Fix bug reading -inf, truncation of integers, and single-quoted control characters, like '[^\n]')
file #35453:  textscan.tst added by lachlan (17KiB - application/octet-stream)
file #35454:  tests_diff.txt added by lachlan (6KiB - text/plain)
file #35357:  textscan_sqbrackets.tst added by philipnienhuis (1KiB - application/octet-stream)
file #35295:  textscan_diff.txt added by None (119KiB - text/plain - Patch relative to 4.0.0 to implement textscan in C++, not .m)

 

Depends on the following items: None found

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by bpabbott (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by None (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-26 mtmiller Carbon-CopyRemoved 80942 => -
    2016-03-19 bpabbott Attached File- => Added textscan-return_on_error-updated.diff, #36694
    2016-03-19 bpabbott Attached File- => Added textscan-return_on_error-updated.diff, #36693
    2016-03-18 philipnienhuis StatusIn Progress => Done
        Assigned toNone => lachlan
        Open/ClosedOpen => Closed
    2016-03-18 lachlan Attached File- => Added textscan-return_on_error.cset, #36680
    2016-03-17 jwe Attached File- => Added textscan-valgrind-log.txt, #36677
    2016-03-15 lachlan Attached File- => Added textscan_diff_default_20160315.txt, #36643
    2016-03-13 philipnienhuis Attached File- => Added ystrread.m, #36626
    2016-03-07 lachlan Attached File- => Added textscan_diff_default_20160307.txt, #36562
    2016-03-05 lachlan Attached File- => Added textscan_diff_default_20160305.txt, #36556
    2016-03-04 lachlan Attached File- => Added ytextscan.cc, #36544
        Attached File- => Added ytextscan.h, #36545
    2016-02-24 philipnienhuis Dependencies- => patch #7969 is dependent
    2016-02-21 lachlan Attached File- => Added ytextscan.cc, #36416
    2016-02-19 lachlan Attached File- => Added ytextscan.cc, #36400
        Attached File- => Added ytextscan.h, #36401
    2016-02-14 lachlan Attached File- => Added textscan_enable_seek_v2.cset, #36334
    2016-02-13 lachlan Attached File- => Added textscan_EOF_handling.cset, #36325
    2016-02-10 lachlan Attached File- => Added textscan_enable_seek.cset, #36288
        Attached File- => Added textscan_module.cset, #36289
    2016-02-04 lachlan Attached File- => Added xtextscan.cc, #36245
        Attached File- => Added xtextscan.h, #36246
    2016-02-03 philipnienhuis Attached File- => Added xtextscan-9jan2016.cs, #36243

    Back to the top


    Powered by Savane 3.5