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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

This would supersede #15756 and fix #15236
@rlwastro please make sure formatting is green (you may comment pre-commit.ci autofix here or fix it locally and push), and please include a changelog entry (see docs/changes/README.rst)

This would supersede #15756 and fix #15236 @rlwastro please make sure formatting is green (you may comment pre-commit.ci autofix here or fix it locally and push), and please include a changelog entry (see docs/changes/README.rst)

I fixed the formatting and added a changelog entry. Let me know if you see other changes that are required.

I'm working on creating a small compressed image that can be used for testing.

@neutrinoceros I'm testing this fix on various images and am beginning to wonder whether I have the correct fix for this issue.

Currently if we read a FITS file with an uncompressed integer image that has a BLANK keyword in the header, the image gets converted from an integer datatype to a float32, and the blanked pixels are set to np.nan. I could argue with that behavior (perhaps a masked integer array would be better than a float32 array). But that is the way it acts now, and as far as I know people are OK with it.

With my version of the code, a FITS file with a compressed integer image that has a BLANK keyword in the header does not get converted to a float. Instead it is kept as an integer array, with the blank values retained as the special integer value.

I think for consistency with the uncompressed image behavior, this should instead convert the array to float32 and replace the blanks with np.nan. If you agree, I can withdraw this pull request and submit a new one. But let me know if you think it needs more discussion before doing that.

This was a bit tricky to discover because the Pan-STARRS images in the original bug report also have BZERO and BSCALE keywords, and it turns out they do in the end get converted to float32 with np.nan for blanks. So the change I'm proposing will in the end give the same result for the PS images. But I'm pretty sure the new approach would be better for other images.

I do think this is important to fix soon! Currently it simply dies with an exception when one of these images is opened. That is clearly bad.

I think for consistency with the uncompressed image behavior, this should instead convert the array to float32 and replace the blanks with np.nan. If you agree, I can withdraw this pull request and submit a new one. But let me know if you think it needs more discussion before doing that.

I can see both ways as satisfying solutions, albeit non ideal. I think @saimn and/or @astrofrog should weigh in here.

@neutrinoceros I figured out the right place to make this change. The code that applies the BSCALE/BZERO scaling and replaces BLANK values with NaN (in compressed.py:CompImageHDU._scale_data()) was skipped if the BSCALE and BZERO values were omitted. It is now executed if BLANK is specified (even in the absence of BSCALE/BZERO).

I updated the description to reflect these additional changes. This version should have the correct behavior (as similar to uncompressed image HDUs as possible).

There is room for improvement to the handling of errors in compressed image headers. I have not tried to make these changes because the code in header.py is pretty scary. My changes do not make these problems any worse at least!

• For integer images, if the ZBLANK header keyword is specified but there is no BLANK keyword, ZBLANK should be copied to BLANK (with a warning).
• For integer images, if neither ZBLANK nor BLANK appears in the header, but there is a ZBLANK column in the table, then BLANK should be set to a default value (with a warning).

@neutrinoceros I made some further improvements in the code that cleaned up some remaining issues with unusual file formats. I believe this patch is complete now.

I added new tests in the test_tiled_compression.py module that I believe should test all the added code. I added some additional (small) test files as well to support those tests. There are still some warnings about code coverage. I'm not sure what the problem is there. I can't tell whether the new tests have actually been run yet or not.

I believe this is very close to complete, so if you or others can help get it through the final stages, that would be very helpful.

Thanks @pllim ! I fixed some filename typos in my test code, and now the patch coverage is 100% complete. All checks now pass.

Alas, we just updated branch protection rules due to matplotlib bump. Would you mind rebasing? Thanks!

@pllim Ah crap, I think I screwed that up. This is very much out of my comfort zone in git! I'll see if I can fix it. If not I may have to start over.

Maybe try this?

git fetch upstream main
git rebase upstream/main

And see if the history looks better before you force push?

Whew, thanks! That did the trick. I think things look like they will be OK now. I'm just waiting to confirm the checks are all still OK.

First of, I want to point out this seems like great work to me, though again I'm no authority when it comes to io.fits, but your work is greatly appreciated.

Since you added tests (which is great) alongside some binary files, it would be important to also include the code that was used to generate them if possible. See #16371 for context.

@neutrinoceros @pllim
I can do that, and I agree it seems like a good idea. But I don't know where to put the script. I tried putting it in the data/ directory, but the pre-commit check complains that it needs an __init__.py file. I'm pretty sure we don't want that. I could put it in tests/, but then pytest might try to run it? I poked around in the astropy directories and did not see any obvious place to use for this sort of thing.

I have the code ready to check in but need some guidance before I can complete the process.

I squashed everything into a single commit and changed the fix to be against the v6.1.x branch. Thanks @neutrinoceros for the helpful tips! I hope everything was done correctly. The files changed look correct. There is a check failure for the change log file. I'm not sure whether that is something I need to fix or not.

I haven't been following the convo. The failure isn't from change log, @dhomeier . It is because this PR is opened against v6.1.x and not main. If that is intentional, then close and re-open the PR to re-run the checks.

So, this means that this patch is only going into the next v6.1 series and WILL NOT be in v7.0.0. Is that correct?

Yes, it is intentional – see #16550 (comment) for the rationale.
Apparently needed reopening to make the pre-CI checks aware of the changed target, thanks!

Thank you for your contribution and your patience throughout our review process, @rlwastro! FITS code is something we approach only with great care and caution. ;-)
Thanks also to everyone who helped get this into Astropy 6!