Skip to content

Commit 1fa5a2a

Browse files
embrayembray
authored
Merge pull request #11108 from embray/issue-5408
[#5408] Don't re-verify invalid FITS cards after their values have been replaced
2 parents de7420e + 867d0ed commit 1fa5a2a

3 files changed

Lines changed: 114 additions & 63 deletions

File tree

‎CHANGES.rst‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,6 +1121,9 @@ astropy.io.ascii
11211121
astropy.io.fits
11221122
^^^^^^^^^^^^^^^
11231123

1124+
- Fix bug where manual fixes to invalid header cards were not preserved when
1125+
saving a FITS file. [#11108]
1126+
11241127
astropy.io.misc
11251128
^^^^^^^^^^^^^^^
11261129

‎astropy/io/fits/card.py‎

Lines changed: 79 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,15 @@ class Card(_Verify):
6262
# This regex helps delete leading zeros from numbers, otherwise
6363
# Python might evaluate them as octal values (this is not-greedy, however,
6464
# so it may not strip leading zeros from a float, which is fine)
65-
_number_FSC_RE = re.compile(r'(?P<sign>[+-])?0*?(?P<digt>{})'.format(
66-
_digits_FSC))
67-
_number_NFSC_RE = re.compile(r'(?P<sign>[+-])? *0*?(?P<digt>{})'.format(
68-
_digits_NFSC))
65+
_number_FSC_RE = re.compile(rf'(?P<sign>[+-])?0*?(?P<digt>{_digits_FSC})')
66+
_number_NFSC_RE = \
67+
re.compile(rf'(?P<sign>[+-])? *0*?(?P<digt>{_digits_NFSC})')
68+
69+
# Used in cards using the CONTINUE convention which expect a string
70+
# followed by an optional comment
71+
_strg = r'\'(?P<strg>([ -~]+?|\'\'|) *?)\'(?=$|/| )'
72+
_comm_field = r'(?P<comm_field>(?P<sepr>/ *)(?P<comm>(.|\n)*))'
73+
_strg_comment_RE = re.compile(f'({_strg})? *{_comm_field}?')
6974

7075
# FSC commentary card string which must contain printable ASCII characters.
7176
# Note: \Z matches the end of the string without allowing newlines
@@ -96,7 +101,7 @@ class Card(_Verify):
96101
# since a greedy match will find a single-quote after
97102
# the comment separator resulting in an incorrect
98103
# match.
99-
r'\'(?P<strg>([ -~]+?|\'\'|)) *?\'(?=$|/| )|'
104+
rf'{_strg}|'
100105
r'(?P<bool>[FT])|'
101106
r'(?P<numr>' + _numr_FSC + r')|'
102107
r'(?P<cplx>\( *'
@@ -111,17 +116,13 @@ class Card(_Verify):
111116
_value_NFSC_RE = re.compile(
112117
r'(?P<valu_field> *'
113118
r'(?P<valu>'
114-
r'\'(?P<strg>([ -~]+?|\'\'|) *?)\'(?=$|/| )|'
119+
rf'{_strg}|'
115120
r'(?P<bool>[FT])|'
116121
r'(?P<numr>' + _numr_NFSC + r')|'
117122
r'(?P<cplx>\( *'
118123
r'(?P<real>' + _numr_NFSC + r') *, *'
119124
r'(?P<imag>' + _numr_NFSC + r') *\))'
120-
r')? *)'
121-
r'(?P<comm_field>'
122-
r'(?P<sepr>/ *)'
123-
r'(?P<comm>(.|\n)*)'
124-
r')?$')
125+
fr')? *){_comm_field}?$')
125126

126127
_rvkc_identifier = r'[a-zA-Z_]\w*'
127128
_rvkc_field = _rvkc_identifier + r'(\.\d+)?'
@@ -725,19 +726,6 @@ def _parse_value(self):
725726
if self._check_if_rvkc(self._image):
726727
return self._value
727728

728-
if len(self._image) > self.length:
729-
values = []
730-
for card in self._itersubcards():
731-
value = card.value.rstrip().replace("''", "'")
732-
if value and value[-1] == '&':
733-
value = value[:-1]
734-
values.append(value)
735-
736-
value = ''.join(values)
737-
738-
self._valuestring = value
739-
return value
740-
741729
m = self._value_NFSC_RE.match(self._split()[1])
742730

743731
if m is None:
@@ -789,21 +777,22 @@ def _parse_comment(self):
789777
if self.keyword in Card._commentary_keywords or self._invalid:
790778
return ''
791779

792-
if len(self._image) > self.length:
793-
comments = []
794-
for card in self._itersubcards():
795-
if card.comment:
796-
comments.append(card.comment)
797-
comment = '/ ' + ' '.join(comments).rstrip()
798-
m = self._value_NFSC_RE.match(comment)
799-
else:
800-
m = self._value_NFSC_RE.match(self._split()[1])
801-
780+
valuecomment = self._split()[1]
781+
m = self._value_NFSC_RE.match(valuecomment)
782+
comment = ''
802783
if m is not None:
803-
comment = m.group('comm')
804-
if comment:
805-
return comment.rstrip()
806-
return ''
784+
# Don't combine this if statement with the one above, because
785+
# we only want the elif case to run if this was not a valid
786+
# card at all
787+
if m.group('comm'):
788+
comment = m.group('comm').rstrip()
789+
elif '/' in valuecomment:
790+
# The value in this FITS file was not in a valid/known format. In
791+
# this case the best we can do is guess that everything after the
792+
# first / was meant to be the comment
793+
comment = valuecomment.split('/', 1)[1].strip()
794+
795+
return comment
807796

808797
def _split(self):
809798
"""
@@ -817,6 +806,34 @@ def _split(self):
817806
else:
818807
image = self.image
819808

809+
# Split cards with CONTINUE cards
810+
if len(self._image) > self.length:
811+
values = []
812+
comments = []
813+
keyword = None
814+
for card in self._itersubcards():
815+
kw, vc = card._split()
816+
if keyword is None:
817+
keyword = kw
818+
819+
# Should match a string followed by a comment; if not it
820+
# might be an invalid Card, so we just take it verbatim
821+
m = self._strg_comment_RE.match(vc)
822+
if not m:
823+
return kw, vc
824+
825+
value = m.group('strg') or ''
826+
value = value.rstrip().replace("''", "'")
827+
if value and value[-1] == '&':
828+
value = value[:-1]
829+
values.append(value)
830+
comment = m.group('comm')
831+
if comment:
832+
comments.append(comment.rstrip())
833+
834+
valuecomment = f"'{''.join(values)}' / {' '.join(comments)}"
835+
return keyword, valuecomment
836+
820837
if self.keyword in self._special_keywords:
821838
keyword, valuecomment = image.split(' ', 1)
822839
else:
@@ -1051,22 +1068,19 @@ def _format_long_commentary_image(self):
10511068
return ''.join(output)
10521069

10531070
def _verify(self, option='warn'):
1054-
self._verified = True
1055-
1056-
errs = _ErrList([])
1071+
errs = []
10571072
fix_text = f'Fixed {self.keyword!r} card to meet the FITS standard.'
10581073

10591074
# Don't try to verify cards that already don't meet any recognizable
10601075
# standard
10611076
if self._invalid:
1062-
return errs
1077+
return _ErrList(errs)
10631078

10641079
# verify the equal sign position
10651080
if (self.keyword not in self._commentary_keywords and
10661081
(self._image and self._image[:9].upper() != 'HIERARCH ' and
10671082
self._image.find('=') != 8)):
1068-
errs.append(self.run_option(
1069-
option,
1083+
errs.append(dict(
10701084
err_text='Card {!r} is not FITS standard (equal sign not '
10711085
'at column 8).'.format(self.keyword),
10721086
fix_text=fix_text,
@@ -1085,8 +1099,7 @@ def _verify(self, option='warn'):
10851099
keyword = self._split()[0]
10861100
if keyword != keyword.upper():
10871101
# Keyword should be uppercase unless it's a HIERARCH card
1088-
errs.append(self.run_option(
1089-
option,
1102+
errs.append(dict(
10901103
err_text=f'Card keyword {keyword!r} is not upper case.',
10911104
fix_text=fix_text,
10921105
fix=self._fix_keyword))
@@ -1096,8 +1109,7 @@ def _verify(self, option='warn'):
10961109
keyword = keyword.split('.', 1)[0]
10971110

10981111
if not self._keywd_FSC_RE.match(keyword):
1099-
errs.append(self.run_option(
1100-
option,
1112+
errs.append(dict(
11011113
err_text=f'Illegal keyword name {keyword!r}',
11021114
fixable=False))
11031115

@@ -1107,35 +1119,39 @@ def _verify(self, option='warn'):
11071119
# For commentary keywords all that needs to be ensured is that it
11081120
# contains only printable ASCII characters
11091121
if not self._ascii_text_re.match(valuecomment):
1110-
errs.append(self.run_option(
1111-
option,
1122+
errs.append(dict(
11121123
err_text='Unprintable string {!r}; commentary cards may '
11131124
'only contain printable ASCII characters'.format(
11141125
valuecomment),
11151126
fixable=False))
11161127
else:
1117-
m = self._value_FSC_RE.match(valuecomment)
1118-
if not m:
1119-
errs.append(self.run_option(
1120-
option,
1121-
err_text='Card {!r} is not FITS standard (invalid value '
1122-
'string: {!r}).'.format(self.keyword, valuecomment),
1123-
fix_text=fix_text,
1124-
fix=self._fix_value))
1128+
if not self._valuemodified:
1129+
m = self._value_FSC_RE.match(valuecomment)
1130+
# If the value of a card was replaced before the card was ever
1131+
# even verified, the new value can be considered valid, so we
1132+
# don't bother verifying the old value. See
1133+
# https://github.com/astropy/astropy/issues/5408
1134+
if m is None:
1135+
errs.append(dict(
1136+
err_text=f'Card {self.keyword!r} is not FITS standard '
1137+
f'(invalid value string: {valuecomment!r}).',
1138+
fix_text=fix_text,
1139+
fix=self._fix_value))
11251140

11261141
# verify the comment (string), it is never fixable
11271142
m = self._value_NFSC_RE.match(valuecomment)
11281143
if m is not None:
11291144
comment = m.group('comm')
11301145
if comment is not None:
11311146
if not self._ascii_text_re.match(comment):
1132-
errs.append(self.run_option(
1133-
option,
1134-
err_text=('Unprintable string {!r}; header comments '
1135-
'may only contain printable ASCII '
1136-
'characters'.format(comment)),
1147+
errs.append(dict(
1148+
err_text=f'Unprintable string {comment!r}; header '
1149+
'comments may only contain printable '
1150+
'ASCII characters',
11371151
fixable=False))
11381152

1153+
errs = _ErrList([self.run_option(option, **err) for err in errs])
1154+
self._verified = True
11391155
return errs
11401156

11411157
def _itersubcards(self):

‎astropy/io/fits/tests/test_header.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2398,6 +2398,38 @@ def test_strip(self):
23982398
hdr = hdr.copy(strip=True)
23992399
assert set(hdr) == {'HISTORY', 'FOO'}
24002400

2401+
def test_update_invalid_card(self):
2402+
"""
2403+
Regression test for https://github.com/astropy/astropy/issues/5408
2404+
2405+
Tests updating the value of a card that is malformatted (with an
2406+
invalid value literal).
2407+
2408+
This tests two ways of reproducing the problem, one working with a
2409+
Card object directly, and one when reading/writing a header containing
2410+
such an invalid card.
2411+
"""
2412+
2413+
card = fits.Card.fromstring('KW = INF / Comment')
2414+
card.value = 'FIXED'
2415+
assert tuple(card) == ('KW', 'FIXED', 'Comment')
2416+
card.verify('fix')
2417+
assert tuple(card) == ('KW', 'FIXED', 'Comment')
2418+
2419+
card = fits.Card.fromstring('KW = INF')
2420+
hdu = fits.PrimaryHDU()
2421+
# This is a loophole to write a header containing a malformatted card
2422+
card._verified = True
2423+
hdu.header.append(card)
2424+
hdu.header.tofile(self.temp('bogus.fits'))
2425+
2426+
with fits.open(self.temp('bogus.fits')) as hdul:
2427+
hdul[0].header['KW'] = -1
2428+
hdul.writeto(self.temp('bogus_fixed.fits'))
2429+
2430+
with fits.open(self.temp('bogus_fixed.fits')) as hdul:
2431+
assert hdul[0].header['KW'] == -1
2432+
24012433

24022434
class TestRecordValuedKeywordCards(FitsTestCase):
24032435
"""

0 commit comments

Comments
 (0)