DM-16386: Update makeLimitedFitsHeader to force floats to correct type #414
Conversation
timj
force-pushed
the
tickets/DM-16386
branch
2 times, most recently
from
eab88a0
to
324260a
Compare
tests/test_makeLimitedFitsHeader.py
Outdated
| while start < len(header): | ||
| end = start + 80 | ||
| # Strip trailing whitespace to make the diff clearer | ||
| with self.subTest("Testing individual headers"): |
There was a problem hiding this comment.
I strongly suggest you provide the from and to strings as values for subTest instead of "Testing individual headers", e,g.
this = header[start:end].rstrip()
expected = expectedHeader[start:end].rstrip()
with self.subTest(this=this, expected=expected):
...
That way you can just use assertEqual without a message (which is one big advantage to subTest).
I am also tempted to suggest you use astropy.io.fits for header comparison. The advantage is that the code would be more general (it would compare whether two headers were logically identical, without worrying about details that don't matter to FITS). That said, this code is simple enough and avoids using a 3rd party package, and since as you expect the headers to be truly identical then this is certainly acceptable and perhaps preferable.
tests/test_makeLimitedFitsHeader.py
Outdated
| ] | ||
| expectedHeader = "".join("%-80s" % val for val in expectedLines) | ||
|
|
||
| self.assertHeadersEqual(header, expectedHeader, rtol=1e-7) |
There was a problem hiding this comment.
consider an rtol based on floating point information e.g. rtol=np.finfo(np.float32).resolution here and for double precision -- just to make it a bit clearer why you chose these values of rtol. A brief code comment would probably suffice, but finfo makes it truly obvious.
timj
force-pushed
the
tickets/DM-16386
branch
from
324260a
to
472762f
Compare
| std::shared_ptr<ast::FitsChan> getFitsChanFromPropertyList(daf::base::PropertySet& metadata, | ||
| std::set<std::string> const& excludeNames = {}, | ||
| std::string options = ""); | ||
|
|
There was a problem hiding this comment.
I suggest you return a shared ptr instead of a FitsChan. Return value optimization should prevent copying and there is only one kind of FitsChan so polymorphism is not an issue. Or am I missing something?
There was a problem hiding this comment.
I was doing what was done with getPropertyListFromFitsChan.
There was a problem hiding this comment.
PropertyList is polymorphic so pointers are important. Not so for FitsChan.
There was a problem hiding this comment.
I withdraw the suggestion. Channels cannot be copied so it's safest not to pretend that's what we are doing here (even though it would not really be copied). I'm not even sure it would compile. What you have is likely better than what I suggested.
There was a problem hiding this comment.
I'm pretty sure this is a move, not a copy (and FitsChan does seem to be movable). I think the argument that this would have been an elided copy would have been true in C++98 but is not true in C++11.
There was a problem hiding this comment.
@TallJimbo so do you have a preference? I think returning a pointer is a bit less obvious and expected, but don't have strong feelings about it.
There was a problem hiding this comment.
Slight preference for returning by value.
There was a problem hiding this comment.
that would be dropping all the shared pointer stuff and returning ast::FitsChan directly?
Without this "0.0" becomes "0" which is an integer type in FITS.
* Factor out comparison code. * Check that floating point values are numerically the same rather than relying on the string forms to be identical. * Add test for single precision floats.
Rather than creating a FITS header string from the PropertyList and then parsing it to create the FitsChan, construct the FitsChan directly from each PropertyList entry (the type is then known direclty).
timj
force-pushed
the
tickets/DM-16386
branch
from
b828631
to
dfe7ff1
Compare
In FITS a "0" is an integer so we must ensure that "0.0" becomes a floating point number when written as a FITS header card. This PR also includes some improvements to the tests and a fix to booleans.