Repository navigation
String-related fixes for FITS tables - #4228
Conversation
|
Having Numpy 1.10 support is important, so I've added v1.0.6 as the milestone for this. It would be nice to release v1.0.6 in conjunction with 1.1 so that both Astropy versions have Numpy 1.10 support concurrently. Otherwise this can be fixed in Astropy 1.1 first, with a note in the v1.0.6 changelog that it will be fixed there too. |
|
@embray - a simultaneous release would be nice, and to be honest this alone would be a good justification for 1.0.6. |
|
Yeah--there are some other things that could slip in to v1.0.6. It's not a big deal to release it either way (at least as long as all the build issues I was having with 1.0.5 stay fixed...) |
|
@embray - had a look to the best of my ability, and think this is all OK. Nice to see you just using the proper way to view rather than my quick hack! |
|
@mhvk I actually did put in your version but it ended up making no difference--looks like i had already fixed all the issues related to that. |
|
Oops, causing a small issue on Numpy 1.9. Should have that fixed in a jiffy... |
|
Ah, only occurs on Windows. That's why I missed it. |
|
Apart from the false positive for coverage, this looks all good. Thanks, @embray! (Not merging yet as I'm not quite sure whether others want to have a look, and how to do the transfer to 1.1.) |
|
Coverage did drop in one file in a relevant manner, but that's because it's code that's normally only run on Python 3. It would be nice if we could collect coverage for Python 3 too. I'm going to write one additional test that should cover that code on both Pythons. Really I should have added such a test in the first place. |
…astropy#4228--should improve the coverage too.
|
Hmm. Coveralls thinks that the |
|
Trying to figure out why that test wasn't covering the cases that I thought it was led me to discover something of a bug, which means in part that this fix isn't making the performance improvements it promises. If this ends up holding things up for too long we can go ahead and merge, because in terms of overall behavior it's correct, and it does fix several bugs and fix Numpy 1.10 support which we need to get out ASAP. But I want to spend a little more time on this... |
|
OK, hope you'll figure it out! And very nice to see numpy 1.10 supported! |
|
I'm not too worried about such reports. It's already been documented as a known issue so people who search ahead of time can find that. Definitely want to spend some more time on this today. |
|
Huh. Strange about the latest test failures. It worked locally. Anyways, it turns out that what I really wanted to do to fix this all properly ended up going down a rabbit hole of everything that's wrong with the old FITS table implementation. I've actually made some real progress on improving things there but it's going to take more time and I need to get this release out before I go on vacation. So I'll get this last test fixed and then merge this. |
|
OK, sounds good. |
|
Okay, the test failure was really only on Python 2, which I sort of expected. It doesn't actually fail on Python 3, but there was something wrong with the build where it ran the Python 3 tests on Python 2. Hopefully that was just a temporary issue...we'll see... |
81771f2 to
214376a
Compare
|
Trying to get the Windows build to run again...for some reason last time the Python 3 test ran fine, but the Python 2 test hanged for no apparent reason. |
…o preserve subtypes when calling .view() with just a dtype argument. However, FITS_rec breaks on non-structured dtypes, so better just play it safe and be explicit when we want to treat one as a raw ndarray).
…makes some of the code a little cleaner. The context manager also makes sure to apply a new dtype to the data while it's byteswapped (turns out this fix is less necessary than I thought it was, but I still think the end result is nicer)
…y-patched version of np.chararray :( I wonder if this was responsible for other subtle bugs that I ended up working around in other ways. Not sure yet... In fixing this it occurred to me that the monkey-patch is no longer necessary in Numpy 1.10 yay!
… when using the list-based constructor. Oh well. See numpy/numpy#2196; I don't think arbitrary offsets is much of an issue here (that only matters for ASCII tables, to which this code doesn't apply).
…. Really the code in this test should apply to any array with a stringy dtype
…think on Numpy 1.10 keyword arguments for view are not supported; go figure)
…astropy#4228--should improve the coverage too.
…sues. In the first case there was a warning that is misleading. The second case (with BinTableHDU.from_columns) actually crashed previously. The easiest fix to this was to just flatten() the inarray first. This is not ideal because flatten() will in many cases result in a copy--for example if the array was a field in a structured array. But best to just get the issue fixed for now (and the regression test working). I want to make some further changes to how TDIM is handled that will maybe improve things here...
…ncorporate the array element shape (as specified by the TDIMn keywords) from the start. My expectation is that this will allow simplification of several parts of the code, where it may be a pain to reshape arrays.
…h that the total number of elements in the TDIM is less than the repeat count of the column format, should be possible in FITS--the trailing bytes are simply ignored. This is the fix needed so that this case can be properly handled. Note that it is NOT possible to have a repeat count that is too low to contain all the elements--this currently results in an exception, and an unreadable table.
…that the _numpy_hacks module was solving, by exploting dtype.__setstate__. So we can throw away my C implementation and just use a simpler (and probably slightly more robust, though still admittedly hacky) pure Python implementation of this. Too bad I did that work for nothing, but it's easier if we don't have to use it :)
…so noted the known issue of astropy#4259 [skip ci]
214376a to
bb60320
Compare
String-related fixes for FITS tables
String-related fixes for FITS tables
String-related fixes for FITS tables
String-related fixes for FITS tables
String-related fixes for FITS tables
String-related fixes for FITS tables
…not sure how this passed on earlier Numpy versions either...)
…not sure how this passed on earlier Numpy versions either...)
…not sure how this passed on earlier Numpy versions either...)
…astropy#4228--should improve the coverage too.
…so noted the known issue of astropy#4259 [skip ci]
…not sure how this passed on earlier Numpy versions either...)
3 participants
This PR fixes a number of string-related issues with FITS tables, especially (but not limited to) Python 3.
This was originally motivated primarily by #3854, and does get all the existing io.fits tests passing when run with Numpy 1.10. However, this required fixing a number of bugs that only started showing up in the tests with Numpy 1.10, but were in many cases real bugs in their own right. It ended up making the most sense to fix them all together, since most of them were related.
chararrayobjects (just normal string arrays). However, there are several areas in which pyfits, and subsequentlyastropy.io.fitsstill rely on thechararrayclass for its behavior of automatically rstripping strings returned from the array (this is relevant for FITS, which dictates that trailing spaces in strings should be ignored). This ensures thatchararrayusage is preserved for now, for backwards compatibility, until another solution can be provided.Tableout to FITS withTable.write(), if the table contained string columns those columns are most likely represented as unicode arrays on Python 3. When writing to a FITS file the unicode was still converted properly to ASCII, but it kept the original width of the unicode arrays. So for example a column of single character strings was written to FITS as a four character column. Getting around this is a bit of a hack for now, and results in slower write times, but at least has the correct behavior. All the more reason for a type of "ASCII only 'unicode'" array._ascii_encodefunction which is improved overnp.char.encodein that it can write to an existing output array. This results in lower peak memory usage when writing out character arrays in some cases.chararrayclass in Numpy >= 1.10, where the issue that prompted that patching is now fixed 😀