Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed a bug that caused compressed images with TFORM missing the optional '1' prefix to not be readable #15001

Merged
merged 4 commits into from Jun 30, 2023
Merged

Fixed a bug that caused compressed images with TFORM missing the optional '1' prefix to not be readable #15001

merged 4 commits into from Jun 30, 2023

Conversation

astrofrog
Copy link
Member

Fixes #14985

@astrofrog astrofrog requested a review from saimn as a code owner June 30, 2023 06:20
@github-actions
Copy link

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 "When to rebase and squash commits".
  • 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?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • 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.

@astrofrog astrofrog mentioned this pull request Jun 30, 2023
Closed
@astrofrog astrofrog added the 💤 backport-v5.3.x on-merge: backport to v5.3.x label Jun 30, 2023
@astrofrog
Copy link
Member Author

pre-commit.ci autofix

@pllim pllim added this to the v5.3.1 milestone Jun 30, 2023
@pllim pllim added the Bug label Jun 30, 2023
saimn
saimn reviewed Jun 30, 2023
View reviewed changes
astropy/io/fits/_tiled_compression/tiled_compression.py Outdated
break
else:
raise RuntimeError(f"Invalid TFORM1: {header['TFORM1']}")
for suffix in range(1, 10):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's possible to have more than 2 columns ? (COMPRESSED_DATA and GZIP COMPRESSED DATA) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One can also have ZSCALE and ZZERO for instance, and I'm not sure if the standard disallows additional columns that the user might have added - but I've now changed this to use TFIELDS to know how many columns there actually are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unlikely that there would be more columns:

10.1.3. Table columns
Two columns in the FITS binary table are defined below to con-
tain the compressed image tiles; the order of the columns in the
table is not significant.
[...]
COMPRESSED DATA – [variable-length; required]
GZIP COMPRESSED DATA [variable-length; optional]

But using TFIELDS seems a fine solution 👍

astropy/io/fits/_tiled_compression/tiled_compression.py
dtype = ">i2"
elif tform[2] == "J":
elif tform[1] == "J":
dtype = ">i4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this if/else block can probably be replaced by tform.recformat.dtype.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this but got some test failures, so I might leave this to another follow-up PR as I need time to investigate what is going wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it's just that it seems we could reuse more code from io.fits.column for the header parsing and maybe for the verification, but fine to keep this for later.

@astrofrog
Copy link
Member Author

pre-commit.ci autofix

saimn
saimn approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @astrofrog !

@saimn saimn merged commit 0208baf into astropy:main Jun 30, 2023
23 of 26 checks passed
@lumberbot-app

This comment was marked as resolved.

Sign in to view
@pllim

This comment was marked as resolved.

Sign in to view
@pllim
Copy link
Member

pllim commented Jul 2, 2023

@astrofrog , you changed the API signature in #14821 . So this cannot be backported cleanly without also backporting that one. I am not sure if I want to make assumptions here, so I'll leave the manual backport to you.

astrofrog pushed a commit to astrofrog/astropy that referenced this pull request Jul 3, 2023
@saimn @astrofrog
Backport PR astropy#15001: Fixed a bug that caused compressed images …
7762840 
…with TFORM missing the optional '1' prefix to not be readable
@astrofrog astrofrog mentioned this pull request Jul 3, 2023
Merged
astrofrog pushed a commit to astrofrog/astropy that referenced this pull request Jul 3, 2023
@saimn @astrofrog
Backport PR astropy#15001: Fixed a bug that caused compressed images …
e55a78d 
…with TFORM missing the optional '1' prefix to not be readable
@astrofrog
Copy link
Member Author

Manual backport PR: #15014

pllim added a commit that referenced this pull request Jul 3, 2023
@pllim
Merge pull request #15014 from astrofrog/auto-backport-of-pr-15001-on…
2cc7c5b 
…-v5.3.x

Backport PR #15001 on branch v5.3.x (Fixed a bug that caused compressed images with TFORM missing the optional '1' prefix to not be readable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saimn saimn saimn approved these changes

Assignees
No one assigned
Labels
Bug io.fits 💤 backport-v5.3.x on-merge: backport to v5.3.x
Projects
None yet
Milestone
v5.3.1
Development

Successfully merging this pull request may close these issues.

V5.3 breaking change reading compressed variable word array
3 participants
@astrofrog @pllim @saimn