-
Notifications
You must be signed in to change notification settings - Fork 1
io.fits.Card: minimal float formatting to prevent truncation (swev-id: astropy__astropy-14508) #31
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Exponent normalization in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] The truncation logic in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] In There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Thanks for the updates—this looks much cleaner. One small suggestion for |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Please add a test case for negative zero to ensure formatting preserves the sign and decimal point (e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Consider adding a test for large positive exponents (e.g., |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import pytest | ||
| from astropy.io import fits | ||
| from astropy.io.fits.verify import VerifyWarning | ||
|
|
||
|
|
||
| def _value_field(card_str): | ||
| """ | ||
| Extract the fixed-width value field (up to 20 characters) after '='. | ||
| For standard FITS cards, the numeric value field is exactly 20 characters | ||
| and right-justified after '= '. HIERARCH cards may omit the space or use | ||
| a shortened value indicator; this helper is intended for standard cards | ||
| in these tests. | ||
| """ | ||
| if '=' not in card_str: | ||
| return '' | ||
| eq = card_str.index('=') | ||
| # After '=' there is typically a space before the field | ||
| start = eq + 1 | ||
| if start < len(card_str) and card_str[start] == ' ': | ||
| start += 1 | ||
| end = min(start + 20, len(card_str)) | ||
| return card_str[start:end] | ||
|
|
||
|
|
||
| def test_hierarch_float_minimal_repr_no_truncate(): | ||
| c = fits.Card( | ||
| 'HIERARCH ESO IFM CL RADIUS', | ||
| 0.009125, | ||
| '[m] radius arround actuator to avoid' | ||
| ) | ||
| with pytest.warns(None) as rec: | ||
| s = str(c) | ||
| # No VerifyWarning | ||
| assert not any(isinstance(w.message, VerifyWarning) for w in rec) | ||
| # Card is exactly 80 chars | ||
| assert len(s) == 80 | ||
| # Value uses minimal representation and comment preserved | ||
| assert '= 0.009125 /' in s | ||
| assert s.endswith('[m] radius arround actuator to avoid') | ||
|
|
||
|
|
||
| def test_float_minimal_repr_exponent_uppercase(): | ||
| c = fits.Card('FOO', 1e-5) | ||
| s = str(c) | ||
| # Uppercase exponent with two digits | ||
| assert 'E-05' in s | ||
| # Value field is right-justified within 20 chars for standard card | ||
| vf = _value_field(s) | ||
| assert len(vf) == 20 | ||
| assert vf.endswith('E-05') | ||
| # Leading spaces indicate right-justification | ||
| assert vf[0] == ' ' | ||
|
|
||
|
|
||
| def test_float_integer_like_has_decimal_point(): | ||
| c = fits.Card('BAR', 2.0) | ||
| s = str(c) | ||
| vf = _value_field(s) | ||
| assert '2.0' in vf | ||
|
|
||
|
|
||
| def test_complex_minimal_repr(): | ||
| c = fits.Card('BAZ', complex(0.009125, -5e-6)) | ||
| s = str(c) | ||
| # Real part minimal, imag part uses normalized small exponent | ||
| assert '0.009125' in s | ||
| assert 'E-06' in s | ||
| vf = _value_field(s) | ||
| # Entire complex literal should fit in the 20-char field in standard cards | ||
| assert len(vf) <= 20 | ||
|
|
||
|
|
||
| def test_hierarch_equal_sign_shortening(): | ||
| # Construct a HIERARCH card near the limit to force '='-shortening logic | ||
| # The specific keyword and value combo aims to exceed by 1 char before shortening | ||
| key = 'HIERARCH ESO VERY LONG KEYWORD TEST' | ||
| comment = 'C' * 30 | ||
| c = fits.Card(key, 0.009125, comment) | ||
| with pytest.warns(None) as rec: | ||
| s = str(c) | ||
| # Shortening occurs without raising errors; card length should be 80 | ||
| assert len(s) == 80 | ||
| # No VerifyWarning | ||
| assert not any(isinstance(w.message, VerifyWarning) for w in rec) | ||
|
|
||
|
|
||
| def test_negative_zero_preserved(): | ||
| c = fits.Card('NEGZERO', -0.0) | ||
| s = str(c) | ||
| vf = _value_field(s) | ||
| # Ensure the sign and decimal point are preserved for -0.0 | ||
| assert '-0.0' in vf | ||
|
|
||
|
|
||
| def test_large_exponent_not_zero_padded(): | ||
| c = fits.Card('BIG', 1e100) | ||
| s = str(c) | ||
| vf = _value_field(s) | ||
| # Exponent should be normalized with uppercase E, explicit sign, and | ||
| # no forced zero-padding beyond the natural width (E+100 here) | ||
| assert 'E+100' in vf | ||
| assert len(vf) <= 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor]
_format_floatcontains an unreachable leftover block at the end referencingvalue_strandstr_len(from the previous implementation). Although it won’t execute due to earlier returns, it’s confusing and risks future maintenance issues. Please remove the dead code entirely.