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

Don't convert float32 to float64 in affine_transform #4452

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Sep 3, 2020

Large (e.g. AIA) maps can take up lots of memory (100MB+). As long as accuracy can be sacrificed, these can be converted from 64 to 32 bit floats to save memory. However, affine_transform casts anything that isn't float64 up to float64. Looking at the git history this seems to be to avoid passing integer dytpes to warp, but I don't think this needs to be done for float32.

This PR changes the casting check to make it so that only integer dtypes are cast to float64, avoiding float32 being cast up and reducing memory usage.

@dstansby dstansby requested a review from a team as a code owner September 3, 2020 13:21
@nabobalis nabobalis added this to the 2.1 milestone Sep 3, 2020
@nabobalis nabobalis added the image Affects the image submodule label Sep 3, 2020
@ayshih
Copy link
Member

ayshih commented Sep 3, 2020

I believe the casting from integer to float is a vestige from when we were normalizing the data array before passing it to warp(), which apparently is no longer needed. warp() should work fine on integer arrays because it will internally convert them to float arrays, although I believe we'll want to use the preserve_range keyword.

In other words, I'd strip out the casting code entirely, add preserve_range=True to warp(), and see if everything still works.

@ayshih
Copy link
Member

ayshih commented Sep 3, 2020

A bit tangential to this PR, but...

#2041 removed the normalization code and cast everything to float64. However, I believe the removal of the normalization code should not have been done. Scikit-image's documentation is quite clear that it always assumes that floats are in the range of [0, 1]. While warp() does appear to work fine when that assumption is violated, we are currently relying on undefined behavior.

I suggest that the normalization code be re-inserted.

sunpy/image/transform.py Outdated Show resolved Hide resolved
@dstansby
Copy link
Member Author

dstansby commented Sep 4, 2020

I had a play around with suggestions, and decided they were more complicated than they seem to implement. In the interests of fixing the original bug, and maintaining other behaviour so this can be backported to 2.0.x, I have kept this simple and opened #4453.

@nabobalis
Copy link
Contributor

Failures are server related.

@nabobalis nabobalis merged commit 3f17403 into sunpy:master Sep 4, 2020
@dstansby dstansby deleted the float32-transform branch September 4, 2020 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
image Affects the image submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants