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.

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

Jump to bottom

Improved error message for scale units in FITS. #2023

Merged
merged 1 commit into from
Feb 12, 2014
Merged

Improved error message for scale units in FITS. #2023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion astropy/io/fits/connect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from ...table import Table
from ...utils import OrderedDict
from ...utils.exceptions import AstropyUserWarning
from astropy.units.format.fits import UnitScaleError

from . import HDUList, TableHDU, BinTableHDU, GroupsHDU
from .hdu.hdulist import fitsopen as fits_open
Expand Down Expand Up @@ -238,7 +239,15 @@ def write_table_fits(input, output, overwrite=False):
# Set units for output HDU
for col in table_hdu.columns:
if input[col.name].unit is not None:
col.unit = input[col.name].unit.to_string(format='fits')
try:
col.unit = input[col.name].unit.to_string(format='fits')
except UnitScaleError:
scale = input[col.name].unit.scale
raise UnitScaleError(
Copy link
Member

Choose a reason for hiding this comment

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

Just a couple of minor esthetic changes. You only use col_unit once, so I think you could just leave:

col.unit = input[col.name].unit.to_string(format='fits')

since you don't gain anything from defining the intermediate variable col_unit.

For scale, it does make sense to define it because it does help the readability in the exception, but you can move the

scale = ...

line inside the except clause, because it's only needed then. That is:

except:
    scale = ...
    raise UnitScaleError(...)

Copy link
Member

Choose a reason for hiding this comment

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

That had actually been my suggestion in the first place; earlier the exception message I suggested used the unit name in the message, so it was used twice. But now I agree it's not needed more than once.

"The column '{0}' could not be stored in FITS format "
"because it has a scale '({1})' that "
"is not recognized by the FITS standard. Either scale "
"the data or change the units.".format(col.name, str(scale)))

for key, value in input.meta.items():

Expand Down
13 changes: 13 additions & 0 deletions astropy/io/fits/tests/test_connect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from .... import units as u
from .... import log
from ....tests.helper import pytest, catch_warnings
from astropy.units.format.fits import UnitScaleError

PY3 = sys.version_info[0] >= 3
DATA = os.path.join(os.path.dirname(__file__), 'data')
Expand Down Expand Up @@ -221,3 +222,15 @@ def test_masking_regression_1795():
assert np.all(t['c2'].data == np.array(['abc', 'xy ']))
assert_allclose(t['c3'].data, np.array([3.70000007153, 6.6999997139]))
assert np.all(t['c4'].data == np.array([False, True]))

def test_scale_error():
from astropy.table import Table
a = [1, 4, 5]
b = [2.0, 5.0, 8.2]
c = ['x', 'y', 'z']
t = Table([a, b, c], names=('a', 'b', 'c'), meta={'name': 'first table'})
t['a'].unit='percent'
with pytest.raises(UnitScaleError) as exc:
t.write('t.fits',format='fits', overwrite=True)
assert exc.value.args[0]=="The column 'a' could not be stored in FITS format because it has a scale '(1.0)' that is not recognized by the FITS standard. Either scale the data or change the units."

7 changes: 6 additions & 1 deletion astropy/units/format/fits.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@
from . import utils
from ...utils.misc import did_you_mean

class UnitScaleError(ValueError):
"""
Used to catch the errors involving scaled units,
which are not recognized by FITS format.
"""

Copy link
Member

Choose a reason for hiding this comment

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

@mdboom - is there a better place to keep this kind of exception? It could be more general than FITS, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and no. The FITS unit format is the only one that doesn't support scale. However, it might make more sense to put it in astropy.units.core along with the other unit exceptions.

class Fits(generic.Generic):
"""
Expand Down Expand Up @@ -119,7 +124,7 @@ def to_string(self, unit):

if isinstance(unit, core.CompositeUnit):
if unit.scale != 1:
raise ValueError(
raise UnitScaleError(
"The FITS unit format is not able to represent scale. "
"Multiply your data by {0:e}.".format(unit.scale))

Expand Down