-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fits thumbnails #91
base: master
Are you sure you want to change the base?
Fits thumbnails #91
Conversation
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
- Coverage 76.43% 76.27% -0.17%
==========================================
Files 24 24
Lines 3981 3966 -15
==========================================
- Hits 3043 3025 -18
- Misses 938 941 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…he alpha channel be intact after a pixel value stretch operation
@@ -868,12 +857,34 @@ def aspil(self): | |||
""" | |||
if self._pil is not None: | |||
return self._pil | |||
if self.mode.try_as_pil() is None: | |||
if self.mode not in (ImageMode.RGB, ImageMode.RGBA): |
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.
Hmm, I think that it's probably better to preserve the original semantics here. PIL does support non-RGB(A) image modes, as listed here, and I think the most sensible behavior for this function is for it to succeed if the data array can be expressed as one of those modes.
Also, I forgot to mention this before, but as always, deleting the try_as_pil()
function is an API break, which we should try to avoid if there's not a good reason.
@@ -167,6 +167,9 @@ def tile( | |||
else: | |||
self._tile_tan(cli_progress, parallel, **kwargs) | |||
|
|||
for img in self.coll.images(): | |||
self.builder.make_thumbnail_from_other(img) | |||
break |
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.
Stylistically, I like to have blank lines both before and after indenting constructs such as for
, if,
etc.
array = np.copy(self._array) | ||
array[..., :3] = (array[..., :3] - pixel_cut_low) / ( | ||
pixel_cut_high - pixel_cut_low | ||
) * 255 + 0.5 |
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.
Hmm, now that I think about this part more, I think it will do the wrong thing in some important circumstances.
First. we should always use self.asarray()
instead of directly accessing self._array
.
But second, if self._pil
is not None, maybe we should return it directly? That won't be helpful if the image has an annoying PIL mode like F
, though. So maybe this function should really be called coerce_into_rgb
(tying in to my previous comment about the semantics of the aspil()
method).
Third, if the data array is two-dimensional — which is going to be the most common usage — the array[..., :3]
slice will select only the first three columns of the image. The bulk of the array data won't get scaled correctly. Due to the existence of the F16x3
image mode, I think this function will need to branch its behavior depending on whether
the array's shape looks like (H, W)
or (H, W, 3)
. The only (H, W, 4)
mode that we have is RGBA.
But fourth, I think we also need to think about whether this function should return either RGB or RGBA, or choose one. It it can return RGBA, we'd need to initialize the alpha channel appropriately, setting it to zero for NaNs and the like. But WWT thumbnails must be delivered as RGB JPEGs. So for the proximate use case, we need to drop down the alpha channel eventually.
So, synthesizing all of that, I think that this function should actually explicitly be coerce_to_rgb_pil()
, with logic to fill in all three channels equally for formats like F32. Non-finite pixels (~np.isfinite(array)
, which includes NaNs) should be mapped to black. If there is already a _pil
representation, it should be returned if it is in RGB
mode, or if it is in RGBA
mode, it should be downconverted sensibly (there's probably a PIL function do this?). Other PIL modes should be handled with the asarray()
codepath.
@@ -1205,7 +1214,7 @@ def save( | |||
if format in PIL_RGB_FORMATS and mode is None: | |||
mode = ImageMode.RGB | |||
if mode is not None: | |||
pil_image = pil_image.convert(mode.try_as_pil()) | |||
pil_image = pil_image.convert(mode.value) |
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.
I'm a little wary of dropping the check here since there are ImageMode values that are not valid inputs to Image.convert()
. We need to check for those at some point.
Enabled creation of thumbnails from non PIL formats.
For FITS files we are using linear stretch and the pixel cut values calculated from the tiling process.
try_as_pil
, but maybe I just have trouble imagining when it would be useful.array = (self._array - pixel_cut_low) / ( pixel_cut_high - pixel_cut_low ) * 255 + 0.5
without changing the alpha channel, if there is one. Do you happen to know what I'm looking for, I seem to be googling the wrong thing. Otherwise, I'll look into this again later.