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

Fix xrimage alpha creation in finalize #35

Merged
merged 2 commits into from
Dec 20, 2018

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Dec 20, 2018

This is to fix the huge bug in trollimage 1.6.1. Right now it is producing seemingly completely transparent images. The solution that works here is not the prettiest but it should work for the supported cases. There is another possibility but I'm not sure it is any better. Here are the two cases that are in conflict and the preferred series of operations to complete these tasks:

  1. Add alpha band
    a. Create alpha band from "null" values
    b. Scale data to output data type including scale the alpha band from 0-1 to dtype_min to dtype_max
    c. Cast to data type

  2. Fill NA values with fill_value
    a. Scale data to output data type
    b. Fill "null" values with fill_value (after casting it to output data type)
    c. Cast to data type

If you change these operations around to support only one of these cases it results in either an all 1 alpha band or a fill_value that does not represent the original user provided one.

Best long term solution would be to modify fill_or_alpha to accept a dtype kwarg and absorb some of this functionality (maybe scaling to the dtype). The fill_or_alpha is currently used by the convert method and I'm not sure if any users have started using it (I doubt it), but it is a public method.

@mraspaud Thoughts?

  • Tests added (for all bug fixes or enhancements)
  • Tests passed (for all non-documentation changes)
  • Passes git diff origin/master **/*py | flake8 --diff (remove if you did not edit any Python files)

@coveralls
Copy link

coveralls commented Dec 20, 2018

Coverage Status

Coverage increased (+0.07%) to 87.898% when pulling 3c46754 on djhoese:bugfix-alpha-creation into 8790ced on pytroll:master.

@djhoese
Copy link
Member Author

djhoese commented Dec 20, 2018

Ok, second commit in this PR is a little cleaner. I'm not sure I like the alpha band being outside 0-1 for integer fields, but maybe that's the right thing?

@djhoese
Copy link
Member Author

djhoese commented Dec 20, 2018

The other obvious clean-ish solution to me is to do the series of task for handling the fill value but before casting the array to the output data type, split the alpha band out, scale it to the output data type, join the data array and alpha band again, then cast to the output data type.

Not sure what is best and this still all depends on integer alpha bands being int_min to int_max. I'm not sure that is correct.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud
Copy link
Member

I'm fine with the implement solution right now, but I'm going to check against my category products to see if the alpha works.

@mraspaud
Copy link
Member

Works, merging

@mraspaud mraspaud merged commit 442f94d into pytroll:master Dec 20, 2018
@djhoese djhoese deleted the bugfix-alpha-creation branch December 20, 2018 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants