New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FITS_rec._scale_back(): Incorrectly checks heapsize for TFORMn='Q' #14810
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
Thank you! Since this is a bug fix, it would also need a change log in https://github.com/astropy/astropy/tree/main/docs/changes/io.fits , following instructions laid out by https://github.com/astropy/astropy/blob/main/docs/changes/README.rst . Usually for bug fix, we also recommend adding a regression test though with only enough data to reproduce the problem. Not sure if it is easy to do in your case or not, so I'll leave that to the FITS maintainers to decide. |
Hello! astropy/astropy/io/fits/tests/test_table.py Lines 3207 to 3231 in cc73b24
I think it would be nice to check the fact that for Q format the error is not raised. |
@cmarmo , just for the record, introduced in #13429 ? In that case, existing milestone still stands. Thanks for the clarification! @BillCleveland-USRA , at some point, the commits will need to be squashed, but maybe after adding that test? Thanks! |
Yes, indeed. 🙁 |
@cmarmo When I modified 5.2.2 and used it, I no longer experienced the error while processing large data files. I'm not sure I have time to come up with a regression test for it, but I provided the following instructions on how to test this with our software: Download and install the latest FTOOLS from the HEASARC IXPE python tools uses Astropy to read and process FITS files. Download the data files Run our IXPE utility: ixpedet2j2000.py sc=y clobber=y \
infile=ixpe02001001_det1_evt1_v03.fits.gz \
attitude=ixpe02001001_det1_att_v01.fits.gz \
outfile=det2j2000_out_1.fits |
You might want to reuse my test, modifying the type of the column import numpy as np
from astropy.io import fits
# a matrix with variable length array elements is created
nelem = 2**28
matrix = np.zeros(1, dtype=np.object_)
matrix[0] = np.arange(0.0, float(nelem + 1))
col = fits.Column(name="MATRIX", format=f"QD({nelem})", unit="", array=matrix)
t = fits.BinTableHDU.from_columns([col])
t.name = "MATRIX"
t.writeto("matrix.fits", overwrite=True) this code throws the error and it shouldn't. |
Sure thing. I'll push an update later today. |
f5f428e
to
9e3bfd1
Compare
Added condition type(recformat) == _FormatP to mitigate the issue cause by _FormatQ is a subclass of _FormatP. Added regress test for Issue 14808. Added "what's new" file for pull 14810.
9e3bfd1
to
80edb84
Compare
I added the regression test, and confirmed its validity by:
I believe I've done all the required tasks:
Thanks for your patience while I learn the process. I'm ready to contribute more in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @BillCleveland-USRA for the PR (and @cmarmo for the review :)).
… heapsize for TFORMn='Q'
… heapsize for TFORMn='Q'
… heapsize for TFORMn='Q'
…810-on-v5.2.x Backport PR #14810 on branch v5.2.x (FITS_rec._scale_back(): Incorrectly checks heapsize for TFORMn='Q')
…810-on-v5.3.x Backport PR #14810 on branch v5.3.x (FITS_rec._scale_back(): Incorrectly checks heapsize for TFORMn='Q')
…810-on-v5.0.x Backport PR #14810 on branch v5.0.x (FITS_rec._scale_back(): Incorrectly checks heapsize for TFORMn='Q')
Description
Package: astropy.io.fits
Module: fitsrec.py
If a table is created with a column using TFORMn="Q(...)" and the heapsize becomes greater than 2**31, a ValueError that recommends the user to use the "Q" format is incorrectly raised.
Since _FormatQ is a subclass of _FormatP, the condition at line 1110 if isinstance(recformat, _FormatP): is True for both types. The above heapsize check is performed when 1110 condition is True.
The fix is to modify line 1133 of fitsrec.py to:
This causes the heapsize check to be only performed when recformat's type is _FormatP.
Fixes #14808