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

DM-16386: Update makeLimitedFitsHeader to force floats to correct type #414

Merged
merged 4 commits into from
Nov 20, 2018
Merged

DM-16386: Update makeLimitedFitsHeader to force floats to correct type #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40d2f80
Use FITS boolean for string form not integer
timj Nov 2, 2018
f238fd7
Force decimal point into floating point numbers
timj Nov 2, 2018
b2fae53
Update tests for makeLimitedFitsHeader
timj Nov 2, 2018
dfe7ff1
Construct a FitsChan directly from PropertyList
timj Nov 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions include/lsst/afw/geom/detail/frameSetUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,20 @@ Copy values from an AST FitsChan into a PropertyList
*/
std::shared_ptr<daf::base::PropertyList> getPropertyListFromFitsChan(ast::FitsChan& fitsChan);

/**
Construct AST FitsChan from PropertyList

@param[in] metadata Metadata to transfer; if this is a PropertyList then the order of items is preserved.
@param[in] excludeNames Names of entries to exclude from the returned header string.
@param[in] options Options string to pass to ast::FitsChan constructor.

@return an ast::FitsChan representing this PropertyList

*/
ast::FitsChan getFitsChanFromPropertyList(daf::base::PropertySet& metadata,
std::set<std::string> const& excludeNames = {},
std::string options = "");

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was doing what was done with getPropertyListFromFitsChan.

Copy link
Contributor

Choose a reason for hiding this comment

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

PropertyList is polymorphic so pointers are important. Not so for FitsChan.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@r-owen r-owen Nov 7, 2018 •
edited

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

Slight preference for returning by value.

Copy link
Member Author

Choose a reason for hiding this comment

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

that would be dropping all the shared pointer stuff and returning ast::FitsChan directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

} // namespace detail
} // namespace geom
} // namespace afw
Expand Down
6 changes: 3 additions & 3 deletions src/fits.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,16 @@ std::string makeLimitedFitsHeaderImpl(std::vector<std::string> const &paramNames
out = (boost::format("%-8s= ") % name).str();

if (type == typeid(bool)) {
out += metadata.get<bool>(name) ? "1" : "0";
out += metadata.get<bool>(name) ? "T" : "F";
} else if (type == typeid(std::uint8_t)) {
out += (boost::format("%20d") % static_cast<int>(metadata.get<std::uint8_t>(name))).str();
} else if (type == typeid(int)) {
out += (boost::format("%20d") % metadata.get<int>(name)).str();
} else if (type == typeid(double)) {
// use G because FITS wants uppercase E for exponents
out += (boost::format("%20.15G") % metadata.get<double>(name)).str();
out += (boost::format("%#20.17G") % metadata.get<double>(name)).str();
} else if (type == typeid(float)) {
out += (boost::format("%20.15G") % metadata.get<float>(name)).str();
out += (boost::format("%#20.15G") % metadata.get<float>(name)).str();
} else if (type == typeid(std::string)) {
out += "'" + metadata.get<std::string>(name) + "'";
if (out.size() > 80) {
Expand Down
57 changes: 54 additions & 3 deletions src/geom/detail/frameSetUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,16 @@ std::shared_ptr<ast::FrameSet> readFitsWcs(daf::base::PropertySet& metadata, boo
metadata.remove("RADECSYS");
}

std::string hdr = fits::makeLimitedFitsHeader(metadata, excludeNames);
ast::StringStream stream(hdr);
ast::FitsChan channel(stream, "Encoding=FITS-WCS, IWC=1, SipReplace=0");
auto channel = getFitsChanFromPropertyList(metadata, excludeNames,
"Encoding=FITS-WCS, IWC=1, SipReplace=0, ReportLevel=3");
auto const initialNames = strip ? setFromVector(channel.getAllCardNames()) : std::set<std::string>();
std::shared_ptr<ast::Object> obj;
try {
obj = channel.read();
} catch (std::runtime_error) {
throw LSST_EXCEPT(pex::exceptions::TypeError, "The metadata does not describe an AST object");
}

auto frameSet = std::dynamic_pointer_cast<ast::FrameSet>(obj);
if (!frameSet) {
throw LSST_EXCEPT(pex::exceptions::TypeError,
Expand Down Expand Up @@ -307,6 +307,57 @@ std::shared_ptr<daf::base::PropertyList> getPropertyListFromFitsChan(ast::FitsCh
return metadata;
}

ast::FitsChan getFitsChanFromPropertyList(daf::base::PropertySet& metadata,
std::set<std::string> const& excludeNames,
std::string options) {
// Create FitsChan to receive each of the parameters
auto stream = ast::StringStream("");
auto fc = ast::FitsChan(stream, options);

// Get the parameter names (in order if necessary)
daf::base::PropertyList const *pl = dynamic_cast<daf::base::PropertyList const *>(&metadata);
std::vector<std::string> allParamNames;
if (pl) {
allParamNames = pl->getOrderedNames();
} else {
allParamNames = metadata.paramNames(false);
}

// Loop over the names and add them to the FitsChan if not excluded
for (auto const &fullName : allParamNames) {
if (excludeNames.count(fullName) == 0) {
std::size_t lastPeriod = fullName.rfind(char('.'));
auto name = (lastPeriod == std::string::npos) ? fullName : fullName.substr(lastPeriod + 1);
std::type_info const &type = metadata.typeOf(name);

if (name.size() > 8) {
continue; // The name is too long for a FITS keyword; skip this item
}

if (type == typeid(bool)) {
fc.setFitsL(name, metadata.get<bool>(name));
} else if (type == typeid(std::uint8_t)) {
fc.setFitsI(name, static_cast<int>(metadata.get<std::uint8_t>(name)));
} else if (type == typeid(int)) {
fc.setFitsI(name, metadata.get<int>(name));
} else if (type == typeid(double)) {
fc.setFitsF(name, metadata.get<double>(name));
} else if (type == typeid(float)) {
fc.setFitsF(name, static_cast<double>(metadata.get<float>(name)));
} else if (type == typeid(std::string)) {
std::string str = metadata.get<std::string>(name);
// No support for long strings yet so skip those
if (str.size() <= 68) {
fc.setFitsS(name, metadata.get<std::string>(name));
}
}
}
}
// Rewind the channel
fc.setCard(0);
return fc;
}

} // namespace detail
} // namespace geom
} // namespace afw
Expand Down
80 changes: 75 additions & 5 deletions tests/test_makeLimitedFitsHeader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#

import unittest
import numpy as np

import lsst.utils.tests
from lsst.daf.base import PropertyList
Expand All @@ -29,6 +30,45 @@

class MakeLimitedFitsHeaderTestCase(lsst.utils.tests.TestCase):

def assertHeadersEqual(self, header, expectedHeader, rtol=np.finfo(np.float64).resolution):
"""Compare 80 characters at a time

Floating point values are extracted from the FITS card and compared
as floating point numbers rather than as strings.

Parameters
----------
header : `str`
FITS-style header string calculated by the test.
expectedHeader : `str`
Reference header string.
rtol = `float`, optional
Tolerance to use for floating point comparisons. This parameters
is passed directly to `~lsst.utils.tests.assertFloatsAlmostEqual`.
The default is for double precision comparison.
"""
self.assertEqual(len(header), len(expectedHeader),
msg="Compare header lengths")
start = 0
while start < len(header):
end = start + 80
# Strip trailing whitespace to make the diff clearer
this = header[start:end].rstrip()
expected = expectedHeader[start:end].rstrip()
with self.subTest(this=this, expected=expected):
# For floating point numbers compare as numbers
# rather than strings
if "'" not in expected and ("." in expected or "E" in expected):
nchars = 10
self.assertEqual(this[:nchars], expected[:nchars],
msg=f"Compare first {nchars} characters of '{this}'"
f" with expected '{expected}'")
self.assertFloatsAlmostEqual(float(this[9:]), float(expected[9:]),
rtol=rtol)
else:
self.assertEqual(this, expected)
start += 80

def checkExcludeNames(self, metadata, expectedLines):
"""Check that makeLimitedFitsHeader properly excludes specified names
"""
Expand All @@ -42,7 +82,7 @@ def checkExcludeNames(self, metadata, expectedLines):
header = makeLimitedFitsHeader(metadata, excludeNames=excludeNames)
expectedHeader = "".join("%-80s" % val for val in expectedLines
if val[0:8].strip() not in excludeNames)
self.assertEqual(header, expectedHeader)
self.assertHeadersEqual(header, expectedHeader)

def testBasics(self):
"""Check basic formatting and skipping bad values
Expand All @@ -51,7 +91,11 @@ def testBasics(self):
dataList = [
("ABOOL", True),
("AFLOAT", 1.2e25),
("AFLOAT2", 1.0e30),
("ANINT", -5),
("AFLOATZ", 0.0), # ensure a float stays a float
("INTFLOAT", -5.0),
("LONGFLT", 0.0089626337538440005),
("LONGNAME1", 1), # name is longer than 8 characters; skip it
("LONGSTR", "skip this item because the formatted value "
"is too long: longer than 80 characters "),
Expand All @@ -63,17 +107,43 @@ def testBasics(self):
header = makeLimitedFitsHeader(metadata)

expectedLines = [ # without padding to 80 chars
"ABOOL = 1",
"ABOOL = T",
"AFLOAT = 1.2E+25",
"AFLOAT2 = 1E+30",
"ANINT = -5",
"AFLOATZ = 0.0",
"INTFLOAT= -5.0",
"LONGFLT = 0.0089626337538440005",
"ASTRING1= 'value for string'",
]
expectedHeader = "".join("%-80s" % val for val in expectedLines)

self.assertEqual(header, expectedHeader)
self.assertHeadersEqual(header, expectedHeader)

self.checkExcludeNames(metadata, expectedLines)

def testSinglePrecision(self):
"""Check that single precision floats do work"""
metadata = PropertyList()

# Numeric form of single precision floats need smaller precision
metadata.setFloat("SINGLE", 3.14159)
metadata.setFloat("SINGLEI", 5.0)
metadata.setFloat("SINGLEE", -5.9e20)
metadata.setFloat("EXP", -5e10)

header = makeLimitedFitsHeader(metadata)

expectedLines = [ # without padding to 80 chars
"SINGLE = 3.14159",
"SINGLEI = 5.0",
"SINGLEE = -5.9E+20",
"EXP = -5E+10",
]
expectedHeader = "".join("%-80s" % val for val in expectedLines)

self.assertHeadersEqual(header, expectedHeader, rtol=np.finfo(np.float32).resolution)

def testArrayValues(self):
"""Check that only the final value is used from an array
"""
Expand All @@ -92,14 +162,14 @@ def testArrayValues(self):
header = makeLimitedFitsHeader(metadata)

expectedLines = [ # without padding to 80 chars
"ABOOL = 0",
"ABOOL = F",
"AFLOAT = -5.6",
"ANINT = 1052",
"ASTRING1= 'more'",
]
expectedHeader = "".join("%-80s" % val for val in expectedLines)

self.assertEqual(header, expectedHeader)
self.assertHeadersEqual(header, expectedHeader)

self.checkExcludeNames(metadata, expectedLines)

Expand Down