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
Expose tile shape and compression type on CompImageHDU #14428
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 for your draft pull request! Do you know that you can use |
👍 Why we have anything in FITS order is beyond me, we should add a new kwarg |
8424a45
to
a78e249
Compare
@saimn - this is ready to review if you think we should do it (but happy to discuss this more if you aren't sure!) |
I really dislike a I think "numpy" and "fits" order really just correspond to array Given that the data shape is transposed, I think it makes sense to transpose the tile shape as well; just have to document it clearly (I'd tend to propagate the changes in the internal API as well). |
@mhvk I've updated the internal code to use tile_shape too now. |
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, just a few comments.
elif tile_size: | ||
if tile_size[0] < 4 or tile_size[1] < 4: | ||
elif tile_shape: | ||
if tile_shape[-1] < 4 or tile_shape[-2] < 4: |
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.
HCOMPRESS_1 is for 2D images so I would keep 0 and 1 for clarity, as this is equivalent to the previous code ? (and same thing below)
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.
It's actually possible (and supported in astropy.io.fits) to compress 3D+ data with HCOMPRESS as long as the tile size is 1 for all extra dimensions so unfortunately I think I have to leave it as I have it now?
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.
Ah OK, interesting, all good then.
@saimn - this is now ready for review again |
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.
All good, thanks @astrofrog !
I think it would be useful to expose the compression type and tile size/shape as public properties on
CompImageHDU
- but I have run into an API design issue. The tile_size passed in toCompImageHDU
is in reverse Numpy order, so if we exposed a property such astile_size
as done in this PR it should return what is passed in to theCompImageHDU
(in general this is a good design - if a parameter is passed in, accessing it via a property should give the same result).However, I think most use cases for this would actually be interested more in the actual Numpy tile shape, so it's not clear what the best option is here. There are several options:
tile_shape
which is in Numpy order and deliberately has a different name fromtile_size
tile_size
as input toCompImageHDU
in favor oftile_shape
, which would be in Numpy order, then have atile_shape
property that matches#Personally I find it very confusing that the input tile size is not in Numpy order and would prefer 2. but perhaps this would need a long deprecation phase depending on how widely this is used in the wild.
In principle we could also expose other settings (dithering etc) as properties but I wanted to first see if we can figure this one out.