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
DM-21355: Try to prevent NaN values turning up in FITS headers #488
Conversation
133f5af
to
d3321da
Compare
boost::format("In %s, found NaN in metadata item '%s'") % | ||
BOOST_CURRENT_FUNCTION % name); | ||
// Convert it to FITS undefined | ||
out += " "; |
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.
Is this FITS undefined, or is it "BLANK "
? https://heasarc.gsfc.nasa.gov/docs/fcg/standard_dict.html
It could be that there's another translation layer somewhere else in the bowels of afw, or that I'm misreading something.
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.
FITS undefined (there is a keyword and an =
sign)
BOOST_CURRENT_FUNCTION % name); | ||
// Convert it to FITS undefined | ||
out += " "; | ||
} | ||
} else if (type == typeid(std::nullptr_t)) { | ||
out += " "; |
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.
Hmmm, I guess this is what has always been used in afw
, so I clearly don't have the full picture.
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.
I added support for FITS undefined a few months back.
# This code causes a SEGV without DM-21355 implemented | ||
frameSet1 = readFitsWcs(metadata, strip=False) | ||
self.assertEqual(type(frameSet1), ast.FrameSet) | ||
|
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.
It makes sense that this test SIGABRT
s on master, this was an untested edge case that could lead to an SIGABRT
and it should now be tested (and fixed).
NaN is not allowed in a FITS header so we convert it to an undefined value.
d3321da
to
75ba42d
Compare
The FITS standard doesn't allow NaN as a value. NaNs turning up in a PropertyList can confuse things so this change traps this and either drops the entire item or replaces with FITS undefined value.