Repository navigation
Update astropy.io.fits.CompImageHDU header when setting data - #12058
ConorMacBride wants to merge 2 commits into
Conversation
Signed-off-by: Conor MacBride <[email protected]>
Signed-off-by: Conor MacBride <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].
| raise TypeError('CompImageHDU data has incorrect type:{}; ' | ||
| 'dtype.fields = {}'.format( | ||
| type(data), data.dtype.fields)) | ||
| if hasattr(self, '_data_first_run') and not self._data_first_run: |
There was a problem hiding this comment.
Seems weird to only set a new attribute here...
|
Thanks! If subpackage maintainers think this should move forward, this PR will also need test(s) and change log. And maybe also user-facing doc on how/when to use it. |
|
p.s. If this PR fixes a particular existing GitHub issue, please fill in |
|
Thanks @ConorMacBride for the patch. I think before we move forward on this I want to take a closer look at exactly what the issue is. There are a few known issues with CompImageHDU some of which I was working on fixing a couple weeks ago (but have been taken away from for other things). So I want to be very careful about additional patches we make to this. IIRC |
|
Should this be treated as a bug fix? (just asking in terms of updating the milestone now that we've branched v5.0.x) |
|
Yes, this is a bug fix so moving the milestone to 5.0.1 |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
saimn
added
keep-open
and removed
Close?
|
@ConorMacBride - #15474 is now merged and I think things now work as expected. Please reopen if I missed something. |
|
@ConorMacBride - #15474 is now merged and I think things now work as expected. Please reopen if I missed something. |
Description
This pull request is to address the following unexpected behaviours:
If a HDU is created with e.g.
hdu = astropy.io.fits.CompImageHDU(), when new data is set (e.g.hdu.data = np.random.rand(10, 20, 30))hdu.headerdoes not match the shape of the new data. However, withastropy.io.fits.ImageHDUit would.If the header is updated with the
astropy.io.fits.CompImageHDU._update_header_datamethod,hdu.headeris still not always updated. This is becauseastropy.io.fits.CompImageHDU.headeris a@lazypropertythat caches its first return value.The first commit in this PR implements an
astropy.io.fits.CompImageHDU.update_headermethod (for symmetry withastropy.io.fits.ImageHDU.update_header). This method calls the_update_header_datamethod and clears the cached@lazyproperty. This resolves the second unexpected behaviour above.The second commit in this PR calls the new
astropy.io.fits.CompImageHDU.update_headermethod in theastropy.io.fits.CompImageHDU.datasetter method (just like inastropy.io.fits.ImageHDU). This resolves the first unexpected behaviour above.astropy/io/fits/tests/test_checksum.pyandastropy/io/fits/tests/test_image.pytests are failing when the header is updated while setting data. I'm pretty sure some of these are failing because tests are not expecting manually removed headers such asZNAXISto be re-added when the data attribute is updated. Before I investigate this further, does what I'm trying to do here make sense?*Fixes #
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel.no-changelog-entry-neededlabel.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.