bugGNU Octave - Bugs: bug #54622, test importdata fails in dev...

 
 

bug #54622: test importdata fails in dev octave with windows

Submitted by:  John Donoghue <lostbard>
Submitted on:  Wed 05 Sep 2018 03:53:50 PM UTC  
 
Category:  Test Suite Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Confirmed Assigned to:  None
Originator Name:  JohnD Open/Closed:  Open
Release:  dev Operating System:  Microsoft Windows

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)

Wed 12 Sep 2018 06:40:58 AM UTC, comment #64:

OK, the attached version with makes use of extract_num() is much easier to comprehend. extract_num() and a couple helper routines are moved to lo-utils.cc unaltered. Because extract_num() doesn't handle upper case I and J, I used std::transform to preprocess the convert the whole string with a simply little helper routine. Slightly less efficient, but so be it for now, unless str2double eventually handles I and J as well.

(file #44988)

Dan Sebald <sebald>
Wed 12 Sep 2018 04:00:17 AM UTC, comment #63:

Yes, that str2double.cc is going in the right direction. I too don't want to dig into that right now, even though it might not bee too bad. It is probably better to add the BISTs, such as trying to read

1 + 5I
0j
+j
-j
3 J

etc.

When you posted, I was just about to finish (so I thought) a change in which I added a Boolean fail status to the octave_read_value() function, then ran into an issue because sparse code was using that function in a strange template.

Nonetheless, attached is a patch that works for reading complex numbers, or is something really close. It became pretty repetitive scanning both the real and imaginary portions of numbers, plus there were so many white-space scenarios, etc. that I now wish I would have abandoned it earlier on and used that extract_num() routine.

Well, anyway, attached is "version 1". Let me know what you think.

I should point out that the change seems to accept a white space between the number and the imaginary indicator:

1.5,0+3 j,4

comes out as 0.0000 + 3.0000i. I suppose that this is fine because this is a data file stream, and unlike the interpreter there is no possible confusion with "j" representing some variable.

(file #44986)

Dan Sebald <sebald>
Tue 11 Sep 2018 09:22:17 PM UTC, comment #62:

Not feeling up to the challenge, but there is a function extract_num in str2double.cc which probably does the right thing.

Rik <rik5>
Project Administrator
Tue 11 Sep 2018 09:16:57 PM UTC, comment #61:

The parser has its own rules. See this ancient bug report from 2010 (https://savannah.gnu.org/bugs/?31974). There has been no resolution there.

isalpha is better than tolower because it is checking for a positive assertion. The default, when given bizarre input such as EOF, will be return false.

You are correct that '1+j' does not read correctly. The issue is in dlmread in the final else clause

For a construct like '1+2j' we have already read '1', and determined that the next character ('+') is not a complex number indicator [iIjJ]. We have also determined that it is not an alpha character and hence a corrupt text such as "InfX". So we pass the result to octave_read_double which would read "+2j" and return 2. That gets made in to a Complex value with the original number in x and stored. However, for '1+j', octave_read_double() returns 0 (default) because there is no number in '+j'.

This is definitely a corner case. If there is an easy way to fix it we should. Otherwise, I wouldn't spend to much time on it. Matlab's dlmread function is very specific about the format it accepts for complex numbers

Octave can at least handle whitespace like "1 + 2j".

Rik <rik5>
Project Administrator
Tue 11 Sep 2018 07:31:25 PM UTC, comment #60:

I forgot to fully recompile, so the NaN+Infj case is cleared up with a rebuild. However,

1.5,1+j,4

fails still. On the other hand, the following works

1.5,1+1j,4

So, we have an issue still with dlmread interpretting a standalone i/j/I/J. I'll look into this, but I wonder if perhaps we should be doing some code reuse here with some small routine in the core parser rather than redoing if/then statement.

Dan Sebald <sebald>
Tue 11 Sep 2018 07:08:08 PM UTC, comment #59:

When the input file is

1.5,1+j,4

the importdata result is

with no mention of NA failures of the underlying dlmread. That is, this result shouldn't be 1.0000 but either 1.0000 + 1.0000j or NA and a textdata cell of "1+1j".

So, I don't think this is quite correct or consistent just yet. We need to do Rik's BIST-driven verification here, I think. :-)

Dan Sebald <sebald>
Tue 11 Sep 2018 06:50:32 PM UTC, comment #58:

OK, I see the reasoning behind the "NaN + Infj", I guess. It is being thought of as

Not sure I completely agree with that, but that's fine. I mean, we could make the argument that

whatever.

However, I then tried some things. The attached change to importdata.m BIST works. On the other hand, reading "NaN+Infj" doesn't seem to work. I changed Rik's example file to be:

and I see that the NaN+Infj is not recognized

Do we need to further enhance the if-else block in dlmread.cc?

(file #44984)

Dan Sebald <sebald>
Tue 11 Sep 2018 05:52:20 PM UTC, comment #57:

@Rik: Ah, good catch. I've updated the changeset, attached. Checking against EOF is correct, rather than std::isalpha (next_char)? I mean, isalpha() is going to be "locale-specific template" dependent as well, right? It seems we should avoid that too.

I was going to add a check to letters in the importdata.m file (i.e., change one of the "Inf" to "Infj", but then I found the result odd with respect to Octave command line input:

The above suggests we can input "Infj" in a file, but not at the command line.

Also, in the following:

how is the that first "Inf*j" acquiring a "NaN" in the real element of the number?

(file #44983)

Dan Sebald <sebald>
Tue 11 Sep 2018 05:45:04 PM UTC, comment #56:

I pushed a change to get rid of std::tolower and just check for the four individual letters (https://hg.savannah.gnu.org/hgweb/octave/rev/11f63a33732e).

@John: Is that enough to fix this report?

Rik <rik5>
Project Administrator
Tue 11 Sep 2018 05:18:31 PM UTC, comment #55:

@Dan, sorry - I was reading something incorrectly. With that patch applied, it appears everything is working correctly.

John Donoghue <lostbard>
Project Member
Tue 11 Sep 2018 05:11:37 PM UTC, comment #54:

@Dan: I think it's probably fine to check explicitly for 'i','I','j','J' rather than using tolower. However, we can't move the test for non-finite values up in the if/else if tree because it then fails to read text like 'Infj' to create a complex value with Infinite imaginary part. Attaching a simple test file for that which you can test with

(file #44982)

Rik <rik5>
Project Administrator
Tue 11 Sep 2018 04:49:25 PM UTC, comment #53:

@John: OK, that's a start. I don't understand the tolower() issue. EOF (-1) is supposed to remain EOF. tolower accepts an int, peek returns an int; so the only thing I can conclude is that somehow the, from documentation, "locale-specific template version" comes into play. Maybe a change of fonts or something in one of the BIST in between? Perhaps a compiler or library bug?? But if not using tolower() solves dlmread(), I'm content with that.

You continued writing as if the importdata() still didn't quite make it through, but the sentence ends oddly as though formatting wasn't quite correct in the post (e.g., missing -verbatim-). Did you test with the latest Octave source that includes Rik's fix for the <CR><LF> problem?

Dan Sebald <sebald>
Tue 11 Sep 2018 02:37:51 PM UTC, comment #52:

With the patch from comment #50, I get a consistent result of:

d = dlmread('c:\octave\num.txt', '\t', 0,0, "emptyvalue", NA)

d =

3.1000 Inf NA
-Inf NaN 128.0000

Note the diffence of Nan vs NA, hoever running test importdata now passes.

But:

>> d = importdata ('c:\octave\num.txt', '\t')

d =

3.1000 Inf NA
-Inf NaN 128.0000

>>

John Donoghue <lostbard>
Project Member
Tue 11 Sep 2018 12:40:19 PM UTC, comment #51:

When displaying the peek char and next char, it does indeed appear that it is reading a peekchar or -1, and it is becoming 255 after the tolower when its failing.

I am trying the patch from comment #50

John Donoghue <lostbard>
Project Member
Mon 10 Sep 2018 10:54:59 PM UTC, comment #50:

Nah, that character after "Inf" must be a x09. Clearly, std::isalpha (next_char) thinks that 255 is alphanumeric for John's system. Depends on the locale, I guess:

http://www.cplusplus.com/reference/cctype/isalpha/

The way I understand this routine is that some error bit during the double conversion such that peek() returns an EOF value:

http://www.cplusplus.com/reference/istream/istream/peek/
http://www.cplusplus.com/reference/ios/ios/rdstate/

Perhaps it would be better to move that condition to the top if-statement. Also, looking at this, perhaps we should just avoid using the tolower() routine. Of interest is only the i,j,I,J letters and it is only two extra machine cycles to if I and J are present. Whereas, with tolower() there needs to be a call/jump/return via a library routine, and that library routine is going to have to check some ranges to make sure that the character in question can be converted to some other ASCII. Why do all that just to account for a couple extra characters?

I'm attaching a changeset that might solve the problem by avoiding tolower(). Please give it a try. If it doesn't work, then we'll have to dig deeper.

(file #44972)

Dan Sebald <sebald>
Mon 10 Sep 2018 09:55:47 PM UTC, comment #49:

Sort of need to confirm that the peek() value is in fact the \t character value... If it turns out that peek() produces 0x09 (\t), might it be that tolower() behaves differently depending on the encoding (e.g., UTF)?

Dan Sebald <sebald>
Mon 10 Sep 2018 09:19:02 PM UTC, comment #48:

"I wrote a quick C++ program and tolower(-2) returned 254 which is weird." -2 in two's complement is the same bit representation as 254 unsigned. Bitwise, arithmetic is pretty much the same, just how one treats the sign bit:

I think you are right about the lolower() not being defined. Perhaps the ASCII test needs to be done prior to the tolower() call to confirm it is in proper range before treating as ASCII.

Dan Sebald <sebald>
Mon 10 Sep 2018 09:04:23 PM UTC, comment #47:

@Rik, your changeset works for my two test cases on Linux, thanks.

@John, you're on the right track. Could you print out the value of x regardless of the case? I.e., I'm assuming that the path taken is this one in the failed scenario:

For some reason that "next_char" is 255 rather than -1 in the failing case. Of course, 255 is the max 8-bit unsigned, and we are talking about next_char being an int, so something is not right in this conversion:

Could you also separate that first line into something like

Dan Sebald <sebald>
Mon 10 Sep 2018 08:59:05 PM UTC, comment #46:

I have answers, but I can at least locate where the problem is occurring. It is in this code

When it works, the tmp_stream.peek() call returns EOF which is -1. When it fails, the next char is 255. One change to make would be to break out the peek and the tolower call. Calling tolower with EOF returns EOF, but using any other negative value is undefined. I wrote a quick C++ program and tolower(-2) returned 254 which is weird. So, perhaps modify

to

Following the code, octave_read_double eventually calls octave_read_fp_value in liboctave/utils/lo-utils.cc.

The trouble may also be here or in read_inf_nan_na().

Rik <rik5>
Project Administrator
Mon 10 Sep 2018 06:31:20 PM UTC, comment #45:

I'll take a look at doing comment #43 as well

John Donoghue <lostbard>
Project Member
Mon 10 Sep 2018 06:29:13 PM UTC, comment #44:

Looking at the issue in dlmread.

I added some print statements to dlmready, but havent had a change to take a real look to see if it tells anything usefull yet:

The on running:

d = dlmread('num.txt', '\t', 0,0, "emptyvalue", NA)

(where num.txt is file #44946)

Running the command before the test suite:

Running it after the test suite was run:

John Donoghue <lostbard>
Project Member
Mon 10 Sep 2018 06:25:53 PM UTC, comment #43:

@John: I can't build a version of Octave for my very old Windows XP 32-bit virtual machine, so I will have to let you try and find this. My guess is that we are doing something exceptional in the test suite that is confusing the internal state of dlmread.

To test this I am uploading two files: idata_LF.txt, tst_dlmread.m. The first file is the data file that fails to load properly. The second file does a dlmread and checks that the data is read correctly. My proposed test sequence is

Work through the directories starting with audio/ and ending with time/. Hopefully you find a failure and then we can narrow it down to a specific script within the directory. If this doesn't process doesn't yield anything then we will need to look at the fixed scripts in the test/ directory.

(file #44969, file #44970)

Rik <rik5>
Project Administrator
Mon 10 Sep 2018 06:17:09 PM UTC, comment #42:

I pushed a patch for the one problem of being unable to read NA with CRLF line ending (https://hg.savannah.gnu.org/hgweb/octave/rev/75f7e8728e17).

Rik <rik5>
Project Administrator
Sat 08 Sep 2018 12:18:48 AM UTC, comment #41:

@Dan: You're right. I think there are at least two overlapping problems. I will probably commit my change for importdata.m since it does fix one of the problems.

I'm trying to cross-compile a version of the dev branch so I can test some of this myself, but it is slow.

Rik <rik5>
Project Administrator
Fri 07 Sep 2018 09:45:08 PM UTC, comment #40:

I think that John might be experiencing a slightly different issue than Rik. See Comment #34, Comment #30 and Comment #4. The importdata.m script does tweak some post-dlmread things, but I can't find anywhere in the code where it re-interprets the Inf and NaN text fields. importdata.m just moves those fields into the textdata return value, so I can't see how anything would correct the NA coming from dlmread. The difference may be in the compiler/C-library used to build on your two systems and how it handles that <istream> >> double extraction operator:

http://www.cplusplus.com/reference/istream/istream/operator%3E%3E/

The issue Rik sees in Windows (and probably John too if only it would get past the dlmread data problem) is something I can replicate on Linux if I force the same scenario.

Dan Sebald <sebald>
Fri 07 Sep 2018 09:01:51 PM UTC, comment #39:

@John: Does it at least pass the test where 'wt' is used to write the data which was previously failing every time?

I've attached that particular case as a standalone script tst_idata.m

(file #44956)

Rik <rik5>
Project Administrator
Fri 07 Sep 2018 06:56:58 PM UTC, comment #38:

With the \r\n, true change, I still get the same BISt error after running the test suite.

John Donoghue <lostbard>
Project Member
Fri 07 Sep 2018 05:43:13 PM UTC, comment #37:

@John: Can you try making this change to importdata.m around line 260?

This passes the BIST test for me.

Rik <rik5>
Project Administrator
Fri 07 Sep 2018 05:33:09 PM UTC, comment #36:

OK, thanks. I see that importdata.m does not go back and reinterpret the Inf and NaN text hunks. So, it is all up to dlmread.cc and that octave_read_double() of Comment #24. I'm wondering why it is then that Windows is going into some kind of mode in which it can no longer read those.

I tried searching for, say, oct-stream that might do the Inf/NaN work, but it appears dlmread.cc is using pretty much the standard C++99 (or higher) steams for which (I think) the Inf and NaN should be interpreted by the standard library code. Is the file being opened in an incorrect mode? E.g., binary rather than text?

It's rather difficult finding where this is all done in the source code without some nice unique keywords to grep for. Anyway, let's approach this by confirming a couple things. If you are able to recompile, I'm attaching a patch that will dump what goes in and comes out of octave_read_double(). The modification is in a CC file so shouldn't cause a long recompile of the project. Please run that and show the result before/after the other BIST tests.

For comparison, here's the kind of output I'm seeing on Linux:

In the above the "missing" STR: is actually the '\r' character moving the line pointer back to the start. (It's kind of unfortunate that the BIST field before the \n is of the NaN variety, but not that important.)

(file #44955)

Dan Sebald <sebald>
Fri 07 Sep 2018 05:24:24 PM UTC, comment #35:

Catching up with this report. I made two versions of the data file using either LF endings or CRLF endings. dlmread is able to read these correctly on Windows.

I can confirm what I guessed, and what Dan found, which is that the problem lies after dlmread in the post-processing around NA.

Rik <rik5>
Project Administrator
Fri 07 Sep 2018 01:49:14 PM UTC, comment #34:

Running initially (before test suite) with the ostrsplit:

Running after the run of test suite:

John Donoghue <lostbard>
Project Member
Fri 07 Sep 2018 08:20:07 AM UTC, comment #33:

I did a little test here with Linux. By adding a \CR to the stream, the problem sort of appears here as well:

But notice how dlmread() does not fail, unlike on the Windows version. In other words, for Linux, dlmread() is programmed to handle either <CR><LF> or just <LF> in an ASCII file. Note that Matlab doc makes no mention of the DOS scenario, so let's assume that our goal is for both Windows and Linux to handle either scenario. Let me fix this here, with the following:

and re-running:

Notice how in the <CR><LF> DOS format the ostrsplit() routine has an extra empty line...which gets tossed by the rest of the routine.

That's my way thinking about this; that we need to handle either Linux or DOS ASCII line endings. So, John, if you apply the diff hunk above, I've a feeling it might work on Windows.

As a secondary note, I notice on Linux, unlike Windows, that dlmread() is properly handling INFINITY and NAN as opposed to just INFINITY for Windows. That is fine because the "touch-up" code that follows addresses these differences. But we still need to answer why it is that in Windows the dlmread() is reading slightly differently for the <CR><LF> and <LF> scenarios. Let's hold off on that until some feedback from John, but jumping ahead a bit, I wonder if instead of the following from dlmread.cc:

which has a linux definition and perhaps slightly different Windows definition with regard to <CR><LF>, to the C++

which might be more consistent between the two, i.e., like the conventional Linux behavior in which \r is lumped in with the resulting data string and it need only be tossed out if present.

Dan Sebald <sebald>
Thu 06 Sep 2018 10:18:33 PM UTC, comment #32:

"The NaN is not processed as expected" meaning that dlmread, although handling +/-Inf, doesn't handle the NaN, so it is the second pass that Rik pointed out which catches the NaN. In other words, neither of those two are working quite right.

Dan Sebald <sebald>
Thu 06 Sep 2018 10:15:53 PM UTC, comment #31:

Oh yeah, that's right. Well, actually looking at the result you posted, it suggests two conflated issues. Note how dlmread() doesn't quite provide the whole interpretation:

The NaN is not processed as expected upon success in

"NA".https://savannah.gnu.org/bugs/index.php?54622#comment5

According to the link

https://stackoverflow.com/questions/1923837/how-to-use-nan-and-inf-in-c

this particular compiler/library must have support for INFINITY then, but not NAN.

Something about the mode when opening or some directive about how escape sequences are treated. Hmm.

Dan Sebald <sebald>
Thu 06 Sep 2018 09:41:33 PM UTC, comment #30:

from comment #20, its happening in the dlmread - return more NA then there should be in the failing tests vs non failing test

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 09:38:49 PM UTC, comment #29:

OK, so maybe it is that on Windows the <CR> alone is left uninterpreted, and only when it is <CR><LF> is it considered "\n". ostrsplit accepts multiple delimiters. Could it be changed to

or might that affect compatibility?

Dan Sebald <sebald>
Thu 06 Sep 2018 09:19:20 PM UTC, comment #28:

I forget, these are stream objects:

http://www.cplusplus.com/reference/fstream/ifstream/close/

"
Note that any open file is automatically closed when the ifstream object is destroyed.
"

That theory is nixed.

Dan Sebald <sebald>
Thu 06 Sep 2018 09:14:33 PM UTC, comment #27:

This is almost surely in the post-processing. I just tested on Windows and I get to this code

The contents of text at this point are "NA\r". On Linux, they would be just "NA". The issue is that strcmpi doesn't pass because of the extra \r.

Rik <rik5>
Project Administrator
Thu 06 Sep 2018 09:14:21 PM UTC, comment #26:

"..., but I don't see an associated "close()" on that file when all done."

Meaning that it could be the dlmread() isn't properly closing the file so the file isn't rewound before "fileread (fname)" of

tries reading in the exact same ASCII that dlmread() processed.

Dan Sebald <sebald>
Thu 06 Sep 2018 09:10:57 PM UTC, comment #25:

Oh, dlmread doesn't read the Inf, etc. and that's done on a second pass. dlmread fills in NA for the empty values. Got it.

There doesn't seem a way of conveniently reading NaN and +-Inf:

https://stackoverflow.com/questions/11420263/is-it-possible-to-read-infinity-or-nan-values-using-input-streams

https://stackoverflow.com/questions/1923837/how-to-use-nan-and-inf-in-c

OK, so continuing where Rik left off...

John, could you delete that semicolon on the first line above in your importdata.m script and check what the file_content looks like? I'm going to guess that maybe it is empty when the test fails.

One thing that concerns me is that I see in the dlmread.cc code that there is an input_file.open(), I don't see an associated "close()" on that file when all done.

Dan Sebald <sebald>
Thu 06 Sep 2018 08:42:21 PM UTC, comment #24:

The dlmread() routine opens a file as follows:

where the second argument is mode. Now, this is a bit more complex than file mode options that I've seen. They are discussed here:

http://www.cplusplus.com/reference/ios/ios_base/openmode/

Notice how none of these are used directly. Instead, the documentation gives some kind of derived class ios_base::in, which also is not the same as std::ios::in. I don't know if the settings are fixed based upon the way std::ios::in is declared, or perhaps based upon the way std::ios::in instance was opened, or what.

From there, it isn't too complicated. The routine just looks at each character to determine if the ascii makes sense in terms of complex number format, etc. and when it determines it should read a double does:

Of course, there can be a lot hidden in that ">>" operator depending on how the stream is opened.

Dan Sebald <sebald>
Thu 06 Sep 2018 08:33:15 PM UTC, comment #23:

for comment 19, the "t" for read makes no difference to the fail (including when the write was also put to text mode)

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 08:26:38 PM UTC, comment #22:

@Dan comment #12:
I think you're right. "test histcounts" in a 1:1000 for loop got me some FAILs.

As it isn't my code I'll first await if the OP of patch #8801 will fix it.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 06 Sep 2018 08:25:06 PM UTC, comment #21:

I think we are narrowing this down to an issue associated only with importdata that is probably in the second half of importdata_ascii.

For speed, importdata first does a dlmread of the file. After that, it goes back over the file to find values that did not convert. In this case, NA, converts to NA and needs to be left alone. Inf/-Inf are exceptional values, but not NA values.

Rik <rik5>
Project Administrator
Thu 06 Sep 2018 08:17:56 PM UTC, comment #20:

continuing from comment 18, using a premade file #44946 and running it before the test suite:

data = dlmread ('test.txt', '\t', 0, 0, "emptyvalue", NA);

returns

3.1000 Inf NA
-Inf NA 128.0000

And after running the test suite:

3.1000 NA NA
NA NA 128.0000

So although it does fail with text files, its not the issue occurring here (although maybe related)

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 08:11:30 PM UTC, comment #19:

Just for grins, I changed to 'wt' on Linux and it works fine.

The same modification fails every time on Windows.

@John, can you try modifying line 159 of importdata to this

The script uses a switch statement on the extension to determine which internal subroutine to use to load the data. If there is no extension, or an unrecognized extension, it goes to importdata_ascii. In this routine, I think we should open the file using 'rt' so that we are sure we are getting text.

Rik <rik5>
Project Administrator
Thu 06 Sep 2018 07:52:43 PM UTC, comment #18:

Comparing a good run vs a fail run, the first time I see if differ in the outputs is at:

output.data = dlmread (fname, delimiter, header_rows, header_cols,
"emptyvalue", NA);

At that point output.data is

fail run:

3.1000 NA NA
NA NA 128.0000

pass run:

3.1000 Inf NA
-Inf NA 128.0000

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 07:34:03 PM UTC, comment #17:

@John, What is the new result when there is the extra character in the file? Same as previous?

Dan Sebald <sebald>
Thu 06 Sep 2018 07:05:43 PM UTC, comment #16:

Of course I may have to take that comment back somewhat - changing it to fid = fopen (fn, "wt"), it fails even without having to run the test suite.

File size of an extra character bigger at that point though, which was not happening on the previously failing files

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 06:59:29 PM UTC, comment #15:

I'll try it, however would have thought it would constantly fail rather than only when/after running the test suit

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 06:53:01 PM UTC, comment #14:

Just a thought, the creation of the file is done with binary mode, despite the fact that this is a text file. The code is

According to the documentation for fopen,

There is an embedded newline '\n' in there which on Linux, in binary mode, is fine because that is the line-ending character. On Windows, however, this probably fails to create a 2-line file. Maybe try altering the BIST test to

and see if things work.

Rik <rik5>
Project Administrator
Thu 06 Sep 2018 06:47:51 PM UTC, comment #13:

The documentation for importdata isn't that great. See Matlab's documentation at http://www.mathworks.com/help/matlab/ref/importdata.html. The field 'textdata' contains row and column headers. For a pure data file like this, I would expect to have textdata be empty.

My guess is that 'NA' which is Octave-specific, is somehow triggering the first row to be treated as text rather than numeric values.

Rik <rik5>
Project Administrator
Thu 06 Sep 2018 06:44:42 PM UTC, comment #12:

@Philip, I see the code. Without looking at it too much, my guess is that these are two different issues. Consider reposting the finding at the histcounts report:

https://savannah.gnu.org/patch/?8801

The thing is, that fail involves random data and it could be that something like the following floor() function isn't quite right with regards to numerical approach:

That is, when the floating point calculation of the bins and normalization is done, it could be that the max_val (or min value) goes epsilon beyond the edge border. Don't know, as I didn't investigate much. Could try simply running the histcounts BIST a hundred times with nothing before or after and see if it fails somewhere with a different random set of data.

Dan Sebald <sebald>
Thu 06 Sep 2018 06:19:10 PM UTC, comment #11:

@Dan:
See comment #1 for a link to the histcounts patch.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 06 Sep 2018 06:12:27 PM UTC, comment #10:

The file is the same in both cases, so it isn't the input file that is the source of the problem.

None of these data reading routines is documented well with regard to the output, in this case A. It's strange that it is creating the expected matrix as a sub-element of a structure, then also some extra bit of information. It appears to me that the extra bit of information is a bunch of strings that the routine cannot translate. That textdata, i.e.,

suggests that importdata, or dlmread, or something else is in a mode in which "Inf" etc. cannot be properly read as float/double values. Perhaps one of the previous texts changed some flag somewhere that controls that.

I find it odd that textdata doesn't have some means of associating the text field with its (possibly multiple) position in the matrix. Since that information is lost, it's not of much use after the import to the user in terms of reconstructing what happened.

Dan Sebald <sebald>
Thu 06 Sep 2018 05:48:21 PM UTC, comment #9:

And for completeness, file #44946 when test passes

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 05:45:08 PM UTC, comment #8:

That was failing file #44945

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 05:43:30 PM UTC, comment #7:

File when test fails attached

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 04:08:03 PM UTC, comment #6:

@Dan: dlmread has BIST tests for it in dlmread.cc.

@John: Can you modify the BIST test to remove the "unlink (fn)" statement and instead use

When it fails, can you go find the actual text file and upload it to this report?

Rik <rik5>
Project Administrator
Thu 06 Sep 2018 12:35:45 PM UTC, comment #5:

On a successful run, the values are:

John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 12:33:21 PM UTC, comment #4:
John Donoghue <lostbard>
Project Member
Thu 06 Sep 2018 06:36:51 AM UTC, comment #3:

histcounts(), is that in a package somewhere? I'm seeing the similar histc(), but no histcounts().

Dan Sebald <sebald>
Thu 06 Sep 2018 06:35:55 AM UTC, comment #2:

dlmread() is now the heart of importdata(), for ASCII. I don't see and BIST for dlmread(), there probably should be.

Could you print out the values of a, d and h? Just copy the lines of code (after the main check) without running the test specifically. I'm curious what 1x1 result a is. Is it all one big string of text perhaps? I can't understand how the algorithm would get stuck reading 3.1 and nothing else beyond that first tab character.

Dan Sebald <sebald>
Wed 05 Sep 2018 07:48:55 PM UTC, comment #1:

Confirmed.

There are more enigmas when running _run_test_suite_ atleast in Windows. E.g., with histcounts.m (see patch #8801), I get two FAILS for histcounts but afterwards:

Maybe there are some hidden variables / functions / whatever not fully cleared from memory?

Surely there's something fishy with importdata itself. After testing it, histcounts first FAILs, but a second try passes all tests:

Philip Nienhuis <philipnienhuis>
Project Member
Wed 05 Sep 2018 03:53:50 PM UTC, original submission:

Running a current version of dev octave in Windows I get a test failure for importdata which doesnt seem to be happening in linux

It failed when running _run_test_suite_, and then afterwards will fail the same running the test separately

Unfortunately, if I run test importdata before testsuite, it passes ok.

John Donoghue <lostbard>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #44982:  tst_infj.txt added by rik5 (11B - text/plain)
file #44969:  idata.LF.txt added by rik5 (23B - text/plain)
file #44970:  tst_dlmread.m added by rik5 (106B - text/x-matlab)
file #44956:  tst_idata.m added by rik5 (282B - text/x-matlab)
file #44946:  oct-YtZpIT added by lostbard (23B - text/plain)
file #44945:  oct-uYuLXw added by lostbard (23B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by lostbard (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 project members can vote.

     

     

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-09-12 sebald Attached File- => Added octave-dlmread_handle_complex-djs2018sep12.patch, #44988
    2018-09-12 sebald Attached File- => Added octave-dlmread_handle_complex-djs2018sep11.patch, #44986
    2018-09-11 sebald Attached File- => Added octave-importdata_bist_infj-djs2018sep11.patch, #44984
    2018-09-11 sebald Attached File- => Added octave-dlmread_no_tolower-djs2018sep11.patch, #44983
    2018-09-11 rik5 Attached File- => Added tst_infj.txt, #44982
    2018-09-10 sebald Attached File- => Added octave-dlmread_no_tolower-djs2018sep10.patch, #44972
    2018-09-10 rik5 Attached File- => Added idata.LF.txt, #44969
        Attached File- => Added tst_dlmread.m, #44970
    2018-09-07 rik5 Attached File- => Added tst_idata.m, #44956
    2018-09-07 sebald Attached File- => Added octave_read_double_dump-djs2018sep07.diff, #44955
    2018-09-06 lostbard Attached File- => Added oct-YtZpIT, #44946
    2018-09-06 lostbard Attached File- => Added oct-uYuLXw, #44945
    2018-09-05 philipnienhuis StatusNone => Confirmed

    Back to the top


    Powered by Savane 3.3