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

Add convert method of XRImage #28

Merged
merged 34 commits into from
Dec 17, 2018
Merged

Conversation

katherinekolman
Copy link

@katherinekolman katherinekolman commented Oct 22, 2018

  • 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 Oct 22, 2018

Coverage Status

Coverage increased (+0.3%) to 86.88% when pulling 45a955a on katherinekolman:master into c8ff27e on pytroll:master.

trollimage/xrimage.py Outdated Show resolved Hide resolved
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.

Looks good.

trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Show resolved Hide resolved
@mraspaud
Copy link
Member

While the Image object implements the convert method as in-place conversion (mainly due to memory constraints), I would like to take the opportunity to change that to convert returning the converted image instead, since with dask, we won't have the same limitation anymore. What do you think ?

trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
@pnuu
Copy link
Member

pnuu commented Nov 22, 2018

While the Image object implements the convert method as in-place conversion (mainly due to memory constraints), I would like to take the opportunity to change that to convert returning the converted image instead, since with dask, we won't have the same limitation anymore. What do you think ?

I'd also like the convertion to return a new image. In this way it would be possible to easily use the same object to get several different colorspaces for what ever use. E.g. to get (in future) YCbCr version and still use the original RGB(A) for something.

@katherinekolman
Copy link
Author

Close #21

The colorize method needed to be updated to run on the numpy array that
was passed, not the buffer of the numpy array.
@pnuu
Copy link
Member

pnuu commented Nov 30, 2018

I tested the updated .colorize() via SatPy (colorized_ir_clouds composite), end the performance boost is quite significant compared to the non-dask version.

trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/tests/test_image.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
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.

Awesome job. There are a couple things I'd like changed.

trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
@djhoese djhoese changed the title Add convert method of xrimage.py Add convert method of XRImage Dec 16, 2018
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.

Starts to shape up nicely!

trollimage/tests/test_image.py Show resolved Hide resolved
trollimage/tests/test_image.py Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Show resolved Hide resolved
trollimage/xrimage.py Outdated Show resolved Hide resolved
trollimage/xrimage.py Show resolved Hide resolved
trollimage/xrimage.py Show resolved Hide resolved
@djhoese djhoese merged commit c89ba18 into pytroll:master Dec 17, 2018
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.

6 participants