Skip to content

Delete scaling keywords from header when unsigned int data converted to another type #5053

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.

Sign up for GitHub

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

Merged
astrofrog merged 3 commits into from
Jun 10, 2016
Merged

Delete scaling keywords from header when unsigned int data converted to another type #5053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3651203
Delete scaling keywords from header when unsigned int data converted …
mwcraig Jun 9, 2016
0627686
Edit test
mwcraig Jun 9, 2016
56d7c87
Add changelog [ci skip]
mwcraig Jun 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,9 @@ Bug fixes

- ``GroupsHDU.is_image`` property is now set to ``False``. [#4742]

- Ensure scaling keywords are removed from header when unsigned integer data
is converted to signed type. [#4974, #5053]

- ``astropy.io.misc``

- ``astropy.io.votable``
Expand Down
85 changes: 80 additions & 5 deletions astropy/io/fits/hdu/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,23 +99,35 @@ def __init__(self, data=None, header=None, do_not_scale_image_data=False,
self._uint = uint
self._scale_back = scale_back

# Keep track of whether BZERO/BSCALE were set from the header so that
# values for self._orig_bzero and self._orig_bscale can be set
# properly, if necessary, once the data has been set.
bzero_in_header = 'BZERO' in self._header
bscale_in_header = 'BSCALE' in self._header
self._bzero = self._header.get('BZERO', 0)
self._bscale = self._header.get('BSCALE', 1)

# Save off other important values from the header needed to interpret
# the image data
self._axes = [self._header.get('NAXIS' + str(axis + 1), 0)
for axis in range(self._header.get('NAXIS', 0))]
self._bitpix = self._header.get('BITPIX', 8)

# Not supplying a default for BITPIX makes sense because BITPIX
# is either in the header or should be determined from the dtype of
# the data (which occurs when the data is set).
self._bitpix = self._header.get('BITPIX')
self._gcount = self._header.get('GCOUNT', 1)
self._pcount = self._header.get('PCOUNT', 0)
self._blank = None if ignore_blank else self._header.get('BLANK')
self._verify_blank()

self._orig_bitpix = self._bitpix
self._orig_blank = self._header.get('BLANK')

# These get set again below, but need to be set to sensible defaults
# here.
self._orig_bzero = self._bzero
self._orig_bscale = self._bscale
self._orig_blank = self._header.get('BLANK')

# Set the name attribute if it was provided (if this is an ImageHDU
# this will result in setting the EXTNAME keyword of the header as
Expand All @@ -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()

@embray embray Jun 10, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@mwcraig mwcraig Jun 10, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, passes locally, will fix in separate PR addressing the remaining comments too.


# Check again for BITPIX/BSCALE/BZERO in case they changed when the
# data was assigned. This can happen, for example, if the input
# data is an unsigned int numpy array.
self._bitpix = self._header.get('BITPIX')

# Do not provide default values for BZERO and BSCALE here because
# the keywords will have been deleted in the header if appropriate
# after scaling. We do not want to put them back in if they
# should not be there.
self._bzero = self._header.get('BZERO')
self._bscale = self._header.get('BSCALE')

@embray embray Jun 10, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally, (and I'm not saying it should be done for this PR, but sooner rather than later) this should be inverted. What's going on is that in some places bzero/bscale are being handled through the header. For example update_header() calls _update_uint_scale_keywords() which in turn sets the correct BSCALE/BZERO for unsigned int data in the header. _update_header_scale_info() then deletes those keywords again and messes with self._bscale and self._bzero`.

Basically what I'm saying is the header should be ignored as early as possible when it comes to managing the state of these attributes. The bzero and bscale should be managed only through the attributes self._bzero, self._bscale, self._orig_bzero, self._orig_bscale (and likewise for bitpix).

self._bzero and self._bzero should at all times contain the values that should go in the header if the header were updated to reflect the current data (which it should be when self.data is set). For unsigned ints this means self._bscale = 1 and self._bzero = <offset>. For floats it typically means bzero=0, bscale=1 unless the data has been explicitly rescaled with scale(). Finally update_header() should only have to look at the current values of self._bscale and self._bzero to update those keywords (as it does self._bitpix).

_orig_bscale and _orig_bzero should only be used for keeping track of the original values they had when the HDU is first initialized with a header (or set to sensible defaults when they were not in the original header). I think this is already fairly consistently the case.

Forgive me if all of the above is obvious to you already. I'm just trying to sort out my own thoughts. This PR looks like a step in the right direction, but there's still a bit of a mess involving management of these attributes that makes the code more complicated and confusing than it needs to be. Not saying that needs to be addressed right this second.

@mwcraig mwcraig Jun 10, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Forgive me if all of the above is obvious to you already.

Not at all -- what I was missing was some sense of what the internal behavior should be, this is very helpful.


# Handle case where there was no BZERO/BSCALE in the initial header
# but there should be a BSCALE/BZERO now that the data has been set.
if not bzero_in_header:
self._orig_bzero = self._bzero
if bscale_in_header:
self._orig_bscale = self._bscale

@embray embray Jun 10, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes sense.


@classmethod
def match_header(cls, header):
"""
Expand Down Expand Up @@ -217,8 +251,10 @@ def data(self, data):
return
else:
self._data_replaced = True
was_unsigned = _is_pseudo_unsigned(self.__dict__['data'].dtype)
else:
self._data_replaced = True
was_unsigned = False

if data is not None and not isinstance(data, np.ndarray):
# Try to coerce the data into a numpy array--this will work, on
Expand All @@ -233,10 +269,11 @@ def data(self, data):
self._modified = True

if isinstance(data, np.ndarray):
# Set new values of bitpix, bzero, and bscale now, but wait to
# revise original values until header is updated.
self._bitpix = DTYPE2BITPIX[data.dtype.name]
self._orig_bitpix = self._bitpix
self._bscale = self._orig_bscale = 1
self._bzero = self._orig_bzero = 0
self._bscale = 1
self._bzero = 0
self._blank = None
self._axes = list(data.shape)
self._axes.reverse()
Expand All @@ -245,7 +282,26 @@ def data(self, data):
else:
raise ValueError('not a valid data array')

# Update the header, including adding BZERO/BSCALE if new data is
# unsigned. Does not change the values of self._bitpix,
# self._orig_bitpix, etc.
self.update_header()
if (data is not None and was_unsigned):
self._update_header_scale_info(data.dtype)

@embray embray Jun 10, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If anything, for unsigned data BZERO and BSCALE should be kept in the header, because that's the only way the unsigned ints can be meaningfully represented in FITS. So in that case keeping them in the header accurately reflects the data (the only reason they are deleted in the generic case is that once the scale factors are applied to the data, they are no longer needed in the header to represent the data unless the data is scaled back before writing).

@mwcraig mwcraig Jun 10, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right; this ends up deleting those keywords only when:

  • The data before this set was unsigned, and
  • The new data is not unsigned.

So BZERO and BSCALE are present when the data is unsigned but are removed when the data is set to something whose type is not unsigned.


# Keep _orig_bitpix as it was until header update is done, then
# set it, to allow easier handling of the case of unsigned
# integer data being converted to something else. Setting these here
# is needed only for the case do_not_scale_image_data=True when
# setting the data to unsigned int.

# If necessary during initialization, i.e. if BSCALE and BZERO were
# not in the header but the data was unsigned, the attributes below
# will be update in __init__.
self._orig_bitpix = self._bitpix
self._orig_bscale = self._bscale
self._orig_bzero = self._bzero


# returning the data signals to lazyproperty that we've already handled
# setting self.__dict__['data']
Expand Down Expand Up @@ -313,11 +369,30 @@ def update_header(self):
if 'BLANK' in self._header:
self._blank = self._header['BLANK']

# Add BSCALE/BZERO to header if data is unsigned int.
self._update_uint_scale_keywords()

self._modified = False

def _update_header_scale_info(self, dtype=None):
"""
Delete BSCALE/BZERO from header if necessary.
"""

# Note that _dtype_for_bitpix determines the dtype based on the
# "original" values of bitpix, bscale, and bzero, stored in
# self._orig_bitpix, etc. It contains the logic for determining which
# special cases of BZERO/BSCALE, if any, are auto-detected as following
# the FITS unsigned int convention.

# Added original_was_unsigned with the intent of facilitating the
# special case of do_not_scale_image_data=True and uint=True
# eventually.
if self._dtype_for_bitpix() is not None:
original_was_unsigned = self._dtype_for_bitpix().kind == 'u'
else:
original_was_unsigned = False

@embray embray Jun 10, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like you were going to do something interesting here but haven't gotten around to it yet. Maybe something to do with my comment here?

Also regarding dtype.kind == 'u' something to be aware of is that this does not apply to 8-bit ints. See #4267 which I believe is still not fixed, but is worth being aware of.


if (self._do_not_scale_image_data or
(self._orig_bzero == 0 and self._orig_bscale == 1)):
return
Expand Down
59 changes: 59 additions & 0 deletions astropy/io/fits/tests/test_image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,65 @@ def test_uint_header_consistency(self):
assert 'BZERO' in new_uint_hdu.header
assert new_uint_hdu.header['BZERO'] == (2 ** (int_size - 1))

@pytest.mark.parametrize(('from_file'), (False, True))
@pytest.mark.parametrize(('do_not_scale'), (False,))
def test_uint_header_keywords_removed_after_bitpix_change(self,
from_file,
do_not_scale):
"""
Regression test for https://github.com/astropy/astropy/issues/4974

BZERO/BSCALE should be removed if data is converted to a floating
point type.

Currently excluding the case where do_not_scale_image_data=True
because it is not clear what the expectation should be.
"""

arr = np.zeros(100, dtype='uint16')

if from_file:
# To generate the proper input file we always want to scale the
# data before writing it...otherwise when we open it will be
# regular (signed) int data.
tmp_uint = fits.PrimaryHDU(arr)
filename = 'unsigned_int.fits'
tmp_uint.writeto(self.temp(filename))
f = fits.open(self.temp(filename),
do_not_scale_image_data=do_not_scale)

uint_hdu = f[0]
# Force a read before we close.
_ = uint_hdu.data
else:
uint_hdu = fits.PrimaryHDU(arr,
do_not_scale_image_data=do_not_scale)

# Make sure appropriate keywords are in the header. See
# https://github.com/astropy/astropy/pull/3916#issuecomment-122414532
# for discussion.
assert 'BSCALE' in uint_hdu.header
assert 'BZERO' in uint_hdu.header
assert uint_hdu.header['BSCALE'] == 1
assert uint_hdu.header['BZERO'] == 32768

# Convert data to floating point...
uint_hdu.data = uint_hdu.data * 1.0

# ...bitpix should be negative.
assert uint_hdu.header['BITPIX'] < 0

# BSCALE and BZERO should NOT be in header any more.
assert 'BSCALE' not in uint_hdu.header
assert 'BZERO' not in uint_hdu.header

# This is the main test...the data values should round trip
# as zero.
filename = 'test_uint_to_float.fits'
uint_hdu.writeto(self.temp(filename))
with fits.open(self.temp(filename)) as hdul:
assert (hdul[0].data == 0).all()

def test_blanks(self):
"""Test image data with blank spots in it (which should show up as
NaNs in the data array.
Expand Down