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

Use correct dtype for RGB image alpha channel #1893

Merged
merged 2 commits into from
Feb 12, 2018
Merged

Conversation

Zac-HD
Copy link
Contributor

@Zac-HD Zac-HD commented Feb 7, 2018

The cause of the bug in #1880 was that I had forgotten to specify the dtype when creating an alpha channel, and therefore concatenating it cast the all the data to float64. I've fixed that, corrected the alpha value for integer arrays, and avoided a pointless copy to save memory.

@fmaussion
Copy link
Member

Thanks @Zac-HD ! Again (and sorry for the hassle ;), this needs a regression test. Also, could you change the rasterio example in the docs and see if your change indeed makes the normalisation obsolete? (https://github.com/pydata/xarray/blob/master/doc/gallery/plot_rasterio_rgb.py#L27)

@Zac-HD Zac-HD changed the title Use correct dtype for RGB image alpla channel Use correct dtype for RGB image alpha channel Feb 9, 2018
# The data is in UTM projection. We have to set it manually until
# https://github.com/SciTools/cartopy/issues/813 is implemented
# https://github.com/SciTools/cartopy/issues/813 is implemented, so that
# the coastlines are drawn in the same coordinate system as the image.
Copy link
Member

Choose a reason for hiding this comment

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

This is not exactly what I meant. Right now we can draw the coastlines in the same coordinate system as the image. What we need SciTools/cartopy#813 for is a tool to convert the PROJ4 string (available as attribute da.crs) to a cartopy.Proj instance (see SciTools/cartopy#813 (comment)). To be honest I don't understand why Cartopy made the choice to create their own new projection classes with their own kwargs instead of relying on an existing system, but this is another topic...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I've just reverted this comment - I don't understand it well enough to improve the explanation, and it's not really part of this PR.

@fmaussion
Copy link
Member

Appart from the comment in the gallery example and the flake8 error, this looks good to me! Thanks @Zac-HD !

Copy link
Member

@fmaussion fmaussion left a comment

Choose a reason for hiding this comment

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

Thanks! I'll leave this open for a couple more days in case someone wants to chime in.

alpha = np.ma.ones(z.shape[:2] + (1,), dtype=z.dtype)
if np.issubdtype(z.dtype, np.integer):
alpha *= 255
z = np.ma.concatenate((z, alpha), 2)
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you use axis=2 here, for clarity? (I loathe undocumented literals)

@fmaussion
Copy link
Member

The failing tests in appveyor should be fixed by #1904

@fmaussion fmaussion merged commit 93a4039 into pydata:master Feb 12, 2018
@fmaussion
Copy link
Member

Thanks @Zac-HD !

@Zac-HD Zac-HD deleted the rgb-maps branch February 14, 2018 05:42
@Zac-HD Zac-HD mentioned this pull request Feb 25, 2018
5 tasks
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.

Should imshow() recognise 0-255 images?
3 participants