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

Fits thumbnails #91

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

imbasimba
Copy link
Member

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.

  1. I don't see a use case for try_as_pil, but maybe I just have trouble imagining when it would be useful.
  2. It seems like there should be an easy way to do this in numpy 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.

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #91 (0284e8e) into master (c1d5d03) will decrease coverage by 0.16%.
The diff coverage is 88.23%.

@@            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     
Impacted Files Coverage Δ
toasty/image.py 86.61% <84.61%> (+0.49%) ⬆️
toasty/builder.py 96.73% <100.00%> (ø)
toasty/fits_tiler.py 47.73% <100.00%> (+0.79%) ⬆️
toasty/toast.py 91.05% <0.00%> (-2.59%) ⬇️
toasty/pyramid.py 92.84% <0.00%> (-0.23%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

toasty/image.py Outdated Show resolved Hide resolved
toasty/image.py Outdated Show resolved Hide resolved
@@ -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):
Copy link

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
Copy link

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
Copy link

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)
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants