Repository navigation
Fix io.ascii test failures on Windows - #3525
Conversation
…ike 'nan'. This particular change probably requires more work (support inf, case-sensitivity, use an ifdef around it?). But this was sufficient as proof-of-concept to fix the tests that were failing because of this.
…tle annoying. But by making the read_ functions into py.test fixtures we can easily take advantage of py.test's tmpdir mechanism that handles temporary files nicely. This was necessary because on my system anyways using NamedTemporaryFile, and then passing the temp file's name to another function that tries to open the file *while it is already open* results in a permission error.
…uld make sure that the comparison text contains the correct line separator as well.
…not confuse editors that are fussy about what to do with tabs."
|
Marked this affects-release, because at least one thing it fixes should be an outright bug, that would result in a crash when trying to use mmap on Windows. |
|
Oh, no I'm realizing that far more of these tests were being skipped on appveyor than I thought. Like, the entire test_c_reader.py module is skipped. After the current test run I'll push another with the appveyor skips turned off. |
|
@embray - oh, no wonder appveyor was passing then! Thanks for digging into this. |
|
I realized this while testing binary releases for Windows. Maybe we should hold off on that until this fix is in? |
|
Hrm. I didn't get this with gcc: :/ |
|
Fixed the NAN issue and went ahead with re-enabling the tests on appveyor as well. Might as well check to see now if they work. |
There was a problem hiding this comment.
@mdmueller @taldcroft Any opinions on this change? Should "inf" be supported too, and what about case-sensitivity. Also it looks like xstrtod doesn't support inf/nan either, though I may be missing something (or maybe it's not supposed to?)
Also, looks like I need to fix a tab here, ironically.
There was a problem hiding this comment.
To be consistent with the Python reader (which in turn uses np.array to do type conversion), this should accept inf as well and be case insensitive (InF should work).
There was a problem hiding this comment.
Should this code also work if the xstrtod is being used? Since it seems like it also doesn't handle inf/nan. I'll have a look at what Numpy does here...
There was a problem hiding this comment.
Nit: It looks like perhaps this new code is introducing tabs? I think we should do 4 spaces, even in C code.
There was a problem hiding this comment.
There was a problem hiding this comment.
Turns out I had vim configured to use tabs in C files already using tabs, and spaces otherwise. Looks like there were several lines in that file that are using tabs already so I'll fix them too.
There was a problem hiding this comment.
Turns out there are quite a number of cases that need to be supported in principle: http://www.gnu.org/software/libc/manual/html_node/Parsing-of-Floats.html Do we really care about the "nan(arbitrary characters)" case? I have never seen this used. Supporting "inf/infinity" and +/- are straightforward and obvious.
|
@embray - apart from my minor comment about I'm curious about your motivation for making all those |
|
It would still be good to get feedback from @mdmueller since I don't have a deep understanding of the C code here. |
|
I haven't looked to carefully at the test changes, but the C/Cython changes look good to me (though I agree with @taldcroft about case insensitivity and |
I agree that was a little weird and don't feel totally confident in that change. Really what that was about is this--I wanted to change the tests to use the pytest-provided temp dir, since that made some issues of dealing properly with temp files easier. But then if I did that I had to go through and change all the In the end I did kind of like the side-benefit of making it easier to see which read function was being used in the test, and in principle to parametrize it. |
|
@embray - thanks for the explanation. Sounds fine to me. I guess this is just waiting on the updates for |
|
Hmm. Well I went ahead and implemented support for strings like |
…not support them (chiefly, Windows). Includes a test to ensure that these strings are treated as floating point values in general. Still sets self->code = CONVERSION_ERROR if the given strings are still unrecognized after the inf/nan check.
There was a problem hiding this comment.
The indentation level doesn't match the logic here. I generally prefer explicit braces in C-like languages for this very reason, though I know that's not the style used elsewhere in this file.
There was a problem hiding this comment.
I'll try to fix these up. This is just after I did a find/replace of tabs for spaces in this file. Depending on one's editor's tab width they previously looked okay, but only because the code was mixing tabs with spaces :(
There was a problem hiding this comment.
I think this counts on long being 32-bits, which isn't true on all platforms. May include stdint.h and use uint32_t?
There was a problem hiding this comment.
On second look, in fairness this only needs to work on Windows platforms and MS compilers, so maybe doesn't matter.
There was a problem hiding this comment.
Yeah this recipe was right out of MSDN.
|
Hrmph. No |
|
It turns out Numpy has its own nice little library of these sorts of string routines. Unfortunately none of it gets exported so we can't easily make use of it. |
… least, if it exists it has another name).
|
I am curious what the inf/nan handling impact on performance will be. Is there an easy way I can test that other than to go ahead and merge and then see what turns up in ASV? I don't really have a good sense of how frequently this is called--and more importantly how frequently this is called on strings that are not floats (and hence goes into the error handling mode). |
bits consistent--I'm not crazy about the braces style used in this module but better to be consistent with it than to fight it :) [skip ci]
8a00b5a to
d57b6f3
Compare
Fix io.ascii test failures on Windows
…new as of ISO C99, and may not be available depending on the compiler and settings. This seems to be a pretty good way of going about defining them in a less platform specific manner (though I keep the definition used by MSVC since it blows up, at least by default, if I try to use 1.0/0.0 as a constant).
Fix io.ascii test failures on Windows Conflicts: astropy/io/ascii/src/tokenizer.c
…new as of ISO C99, and may not be available depending on the compiler and settings. This seems to be a pretty good way of going about defining them in a less platform specific manner (though I keep the definition used by MSVC since it blows up, at least by default, if I try to use 1.0/0.0 as a constant).
…os are new as of ISO C99, and may not be available depending on the compiler and settings. This seems to be a pretty good way of going about defining them in a less platform specific manner (though I keep the definition used by MSVC since it blows up, at least by default, if I try to use 1.0/0.0 as a constant).
5 participants
Fixes many tests for
io.asciithat were failing for me on Windows. Some of these were being skipped on appveyor due to not passing. But I think there were some others that don't explicitly say they are skipped on appveyor, and yet it is a mystery to me how they could have been passing at all...Once this passes the first round of testing I'll take off some of the "skip if appveyor" decorators and see how things go.