You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test_compression_column_tforms failure with cfitsio 3.380 #4646
With the latest relase of cfitsio (which just entered Debian unstable), there are two test failures. The first one:
_________ TestCompressedImage.test_compression_column_tforms ______________
self = <astropy.io.fits.tests.test_image.TestCompressedImage object at 0x7fcf39d96210>
def test_compression_column_tforms(self):
"""Regression test for https://aeon.stsci.edu/ssb/trac/pyfits/ticket/199"""
# Some interestingly tiled data so that some of it is quantized and
# some of it ends up just getting gzip-compressed
data2 = ((np.arange(1, 8, dtype=np.float32) * 10)[:, np.newaxis] +
np.arange(1, 7))
np.random.seed(1337)
data1 = np.random.uniform(size=(6 * 4, 7 * 4))
data1[:data2.shape[0], :data2.shape[1]] = data2
chdu = fits.CompImageHDU(data1, compressionType='RICE_1',
tileSize=(6, 7))
chdu.writeto(self.temp('test.fits'))
with fits.open(self.temp('test.fits'),
disable_image_compression=True) as h:
> assert h[1].header['TFORM1'] == '1PB(30)'
E assert '1PB(45)' == '1PB(30)'
E - 1PB(45)
E + 1PB(30)
astropy/io/fits/tests/test_image.py:1268: AssertionError
This may be a bug in cfitsio as well; I would need your hint here.
Full log here.
This happens on all architectures.
I'll just note that the interface used to CFITSIO is rather fragile (see #3895 for more details). It can easily break with updates to CFITSIO in, in particular since it has to keep up with CFITSIO's internal structures, which it doesn't make much of an attempt at keeping stable or documented in any way.
Looking into the code of cfitsio, I think this is a problem with this test. In cfitsio, the function ffuptf() updates the TFORM keyword with the maximum length, which is actually calculated from the file.
There is no reason why TFORM1 should specify a length of 30 (except that previous versions of cfitsio probably did this) or 45. In principle any length should do it, so maybe the fix could just be testing against the regexp r'1TP\(\d+\)'? What do you think, @embray?
I need to double check, but I think you may be right @olebole. The value that goes in those parens is just meant to be a hint, and there's no requirement that it be any specific value.
The change on the cfitsio side which caused this regression is the following one:
modified the 4 FnNoise5_(type) routines in quantize.c to correctly
count the number of non-null pixels in the input array. Previously the
count could be inaccurate if the image mainly consisted of null pixels.
This could have caused certain floating point image tiles to be
quantized during the image compression process, when in fact the tile
did not satisfy all the criteria to be safely quantized.
Basically the quantization was not done correctly in some cases (including the one in astropy), so fixing this issue changed the resulting compressed file, including the headers.
Therefore the suggested change looks to me the right way to fix the issue.
Activity
astrofrog commentedon Feb 29, 2016
@olebole - what was the previous version in Debian, and did it work correctly?
olebole commentedon Feb 29, 2016
Previous version was 3.370 and it worked correctly.
astrofrog commentedon Feb 29, 2016
@embray - is this something you would still have time to investigate in your spare time, or should one of the other core devs look into it?
embray commentedon Feb 29, 2016
I'll just note that the interface used to CFITSIO is rather fragile (see #3895 for more details). It can easily break with updates to CFITSIO in, in particular since it has to keep up with CFITSIO's internal structures, which it doesn't make much of an attempt at keeping stable or documented in any way.
embray commentedon Feb 29, 2016
#3895 would be a great GSoC project by the way.
olebole commentedon Feb 29, 2016
Looking into the code of cfitsio, I think this is a problem with this test. In cfitsio, the function
ffuptf()updates theTFORMkeyword with the maximum length, which is actually calculated from the file.There is no reason why
TFORM1should specify a length of 30 (except that previous versions of cfitsio probably did this) or 45. In principle any length should do it, so maybe the fix could just be testing against the regexpr'1TP\(\d+\)'? What do you think, @embray?embray commentedon Feb 29, 2016
I need to double check, but I think you may be right @olebole. The value that goes in those parens is just meant to be a hint, and there's no requirement that it be any specific value.
embray commentedon Feb 29, 2016
i have no idea why it would have changed from 30 to 45 though. Curious...
aurel32 commentedon Mar 2, 2016
The change on the cfitsio side which caused this regression is the following one:
Basically the quantization was not done correctly in some cases (including the one in astropy), so fixing this issue changed the resulting compressed file, including the headers.
Therefore the suggested change looks to me the right way to fix the issue.