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

Fixed that save_dataset does not propagate fill_value #423

Merged
merged 2 commits into from Sep 25, 2018

Conversation

TAlonglong
Copy link
Collaborator

Fixed that save_dataset does not propagate fill_value into get_enhanced_image. Further does not get_enhanced_image propagate fill_value into add_overlay and add_decorate. Those in turn does not propagate fill_value into pil_image.

…ed_image. Further does not get_enhanced_image propagate fill_value into add_overlay and add_decorate. Those in turn does not propagate fill_value into pil_image.
@coveralls
Copy link

coveralls commented Sep 24, 2018

Coverage Status

Coverage decreased (-0.02%) to 71.92% when pulling 7da641d on TAlonglong:fix-propagate-fill_value into 078a73a on pytroll:master.

@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #423 into master will not change coverage.
The diff coverage is 20%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   71.94%   71.94%           
=======================================
  Files         128      127    -1     
  Lines       16884    16884           
=======================================
  Hits        12147    12147           
  Misses       4737     4737
Impacted Files Coverage Δ
satpy/writers/__init__.py 76.4% <20%> (ø) ⬆️
satpy/version.py
satpy/__init__.py 94.73% <0%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 078a73a...7da641d. Read the comment docs.

@djhoese
Copy link
Member

djhoese commented Sep 25, 2018

So I've looked at this more and it makes sense that fill_value needs to be passed to the overlay/decorate functions so that pil_image can "finalize" the image properly (adding or not adding an alpha channel as desired). From what I understand the series of events for the bad performing use case is:

  1. Provide L or RGB DataArray to get_enhanced_image
  2. Add decoration or overlay (using associated kwarg to get_enhanced_image.
  3. These call pil_image on the XRImage object which by default will make an 8-bit LA or RGBA DataArray. Which ends up getting returned from get_enhanced_image.
  4. Writer tries to save LA/RGBA data to a JPEG/PNG using the PillowWriter (trollimage.xrimage.XRImage.pil_save). This calls XRImage.finalize which sees that fill_value was provided so it fills any invalid (null) pixels in the DataArray with the fill value but it does not remove the Alpha channel. XRImage assumes that since the user provided an Alpha channel that they want that to still exist. Which I'm guessing fails when saved to JPG and produces undesired output with PNG, right?

So the above situation makes sense for why this is happening and producing undesired results. I have no issue with this solution. I do have an issue with the performance of the functions as implemented. IMO the multiple calls to pil_image and / 255.0 to renormalize the data are wasteful. This of course only happens if an image is decorated and "overlayed" in the same call. I don't want this PR to include a change for this but just wanted to mention it as an issue.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Found a typo


if decorate is not None:
add_decorate(img, **decorate)
add_decorate(img, fille_value=fill_value, **decorate)
Copy link
Member

Choose a reason for hiding this comment

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

Typo fill_value versus fille_value.

@djhoese djhoese merged commit e871ce8 into pytroll:master Sep 25, 2018
@TAlonglong
Copy link
Collaborator Author

Thanks

Regarding the pil_image; yes it is a bad solution if both are invoked. But at the time this was implemented I could not see another solution.

I'm happy to discuss this at some point in time.

@djhoese
Copy link
Member

djhoese commented Sep 26, 2018

Best I could think of is pass the PIL image to these functions. It makes them harder to use outside of the get_enhanced_image function, but not sure that is important. Regardless, it is fine the way it is.

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

Successfully merging this pull request may close these issues.

None yet

3 participants