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.

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

Deprecate automatic changes to the tile size with HCOMPRESS_1 #14410

Merged
merged 3 commits into from Feb 21, 2023

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Feb 17, 2023

When creating a CompImageHDU with a tile size that results in tiles having <4 pixels along one or more dimensions, the tile size is automatically and silently updated - but only if it can be changed by adding +1 to one of the dimensions:

In [1]: import numpy as np

In [2]: from astropy.io import fits

In [4]: hdu = fits.CompImageHDU(np.zeros((50, 50)), compression_type='HCOMPRESS_1', tile_size=[24, 24])

In [5]: hdu._header['ZTILE1']
Out[5]: 25

If the tile size cannot automatically be adjusted, an error is raised:

ValueError: Last tile along 1st dimension has less than 4 pixels

The auto changing is confusing IMHO as it only works in very specific cases (it would have to be that adding 1 makes the tile size acceptable, not removing 1 or not adding 2) and I think we should simply deprecate and eventually remove this behavior in favor of always raising an explicit error, so this PR is to add a deprecation warning to that effect - but this is just a suggestion, I'm happy to be told that the above behavior is desirable! With this PR:

In [3]: hdu = fits.CompImageHDU(np.zeros((50, 50)), compression_type='HCOMPRESS_1', tile_size=[24, 24])
WARNING: AstropyDeprecationWarning: The tile size should be such that no tiles have fewer than 4 pixels. The tile size has automatically been changed from [24, 24] to [25, 25], but in future this will raise an error and the correct tile size should be specified directly. [astropy.io.fits.hdu.compressed]

Note that a side effect of the current behavior is that tuples cannot be passed for the tile size which is also confusing:

In [3]: hdu = fits.CompImageHDU(np.zeros((50, 50)), compression_type='HCOMPRESS_1', tile_size=(24, 24))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 hdu = fits.CompImageHDU(np.zeros((50, 50)), compression_type='HCOMPRESS_1', tile_size=(24, 24))

File ~/Dropbox/Code/Astropy/astropy/astropy/io/fits/hdu/compressed.py:662, in CompImageHDU.__init__(self, data, header, name, compression_type, tile_size, hcomp_scale, hcomp_smooth, quantize_level, quantize_method, dither_seed, do_not_scale_image_data, uint, scale_back, **kwargs)
    654     self.data = data
    656     # Update the table header (_header) to the compressed
    657     # image format and to match the input data (if any);
    658     # Create the image header (_image_header) from the input
    659     # image header (if any) and ensure it matches the input
    660     # data; Create the initially empty table data array to
    661     # hold the compressed data.
--> 662     self._update_header_data(
    663         header,
    664         name,
    665         compression_type=compression_type,
    666         tile_size=tile_size,
    667         hcomp_scale=hcomp_scale,
    668         hcomp_smooth=hcomp_smooth,
    669         quantize_level=quantize_level,
    670         quantize_method=quantize_method,
    671         dither_seed=dither_seed,
    672     )
    674 # TODO: A lot of this should be passed on to an internal image HDU o
    675 # something like that, see ticket #88
    676 self._do_not_scale_image_data = do_not_scale_image_data

File ~/Dropbox/Code/Astropy/astropy/astropy/io/fits/hdu/compressed.py:1110, in CompImageHDU._update_header_data(self, image_header, name, compression_type, tile_size, hcomp_scale, hcomp_smooth, quantize_level, quantize_method, dither_seed)
   1107 original_tile_size = tile_size[:]
   1109 if remain > 0 and remain < 4:
-> 1110     tile_size[0] += 1  # try increasing tile size by 1
   1112     remain = self._image_header["NAXIS1"] % tile_size[0]
   1114     if remain > 0 and remain < 4:

TypeError: 'tuple' object does not support item assignment

@github-actions
Copy link

github-actions bot commented Feb 17, 2023

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.

@pllim pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Feb 17, 2023
@pllim pllim added this to the v5.3 milestone Feb 17, 2023
@pllim
Copy link
Member

pllim commented Feb 17, 2023

CI failures should go away if you rebase.

I am surprised no tests are affected by this change.

@astrofrog
Copy link
Member Author

I don't think there were any tests of this 'feature'

@saimn
Copy link
Contributor

saimn commented Feb 19, 2023

👍 for deprecating and raising an error in the future. There are now some conflicts to resolve.

@astrofrog astrofrog force-pushed the deprecate-hcompress-incorrect-tile branch from 0ca6357 to 692dd41 Compare February 21, 2023 10:32
@astrofrog
Copy link
Member Author

@saimn - rebased and ready for final review once you are back :)

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.

LGTM, thanks @astrofrog.

@saimn saimn merged commit 50909ab into astropy:main Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period io.fits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants