Delete scaling keywords from header when unsigned int data converted to another type #5053
Conversation
…to another type Closes astropy#4974
|
The bscale/bzero handling has always been a mess. I've attempted a couple times to clean it up, and it always just ends up a mess again. I think some deeper refactoring is needed (stuff I had planned to but never had the time). (What I'm saying is, I'm sorry you had to deal with it!) |
| @@ -135,9 +147,31 @@ def __init__(self, data=None, header=None, do_not_scale_image_data=False, | |||
| self._data_needs_rescale = True | |||
| return | |||
| else: | |||
| # Setting data will set set _bitpix, _bzero, and _bscale to the | |||
| # appropriate BITPIX for the data, and always sets _bzero=0 and | |||
| # _bscale=1. | |||
| self.data = data | |||
| self.update_header() | |||
There was a problem hiding this comment.
It looks to me like update_header() already gets called when setting self.data. See https://github.com/mwcraig/astropy/blob/0627686c4c422c7f183e552d70d320a0f1540a5f/astropy/io/fits/hdu/image.py#L288
So this can probably be removed (if somehow things break unless it's called twice then that would be something to look into as well).
Basically most of this business of setting self._bitpix shouldn't be happening here in __init__ at all, and should just happen when setting self.data =... (since that can happen outside of __init__ as well).
There was a problem hiding this comment.
Indeed, passes locally, will fix in separate PR addressing the remaining comments too.
|
I think if this fixes the immediate bug and passes all the tests then I think it should be merged (and backported as relevant). But I have a lot of comments above, as you can see, which I think should be addressed. If not by this PR then an issue should be opened for the followup work. |
|
@mwcraig - let me know how you want to proceed. If you would prefer to address the issues now, we could just put this in 1.2.1 instead? |
|
@astrofrog -- let's merge as is. I've got a couple questions about @embray's (very helpful) comments and will start a follow-up PR today for 1.2.1. The underlying bug is fairly serious, though, and should be addressed asap: Reading unsigned data, performing floating point operations on it, then writing it out will result in corrupt data values. |
Not a problem; it sounds like we can work out some kind of sensible internal behavior which I can then implement! |
|
@mwcraig - ok, in that case, can you add a changelog entry, with |
|
@astrofrog -- done. |
Delete scaling keywords from header when unsigned int data converted to another type
Ideally this would get into 1.2 (not necessary into the RC, but before the final release). It addresses #4974 but involved a fair bit of monkeying around with the internals of scaling.
@embray, if you could take a look that would be great.
Changelog as soon as I find out which section to put the entry in....