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-11957: Cannot round-trip >7th degree Chebyshev photometry models #277

Merged
merged 7 commits into from
Oct 3, 2017
Merged

DM-11957: Cannot round-trip >7th degree Chebyshev photometry models #277

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8de7d66
Minor enhancements to test_simpleTable.py
r-owen Sep 21, 2017
c20f022
Add uint8 scalar and array field types
r-owen Sep 21, 2017
abc3b23
Add stringToBytes and bytesToString
r-owen Sep 25, 2017
3d2d45a
Remove an unused library module
r-owen Sep 25, 2017
43af436
Change TransformBoundedField table persistence
r-owen Sep 25, 2017
adf97df
Fix doxygen errors in ImageBase and MaskedImage
r-owen Oct 2, 2017
6ecb45e
Fix doxygen warning in fitsCompression.cc
r-owen Oct 3, 2017
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
12 changes: 6 additions & 6 deletions include/lsst/afw/table/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,20 @@
*/

// Scalar types: those that can serve as elements for other types, and use the default FieldBase template.
#define AFW_TABLE_SCALAR_FIELD_TYPE_N 6
#define AFW_TABLE_SCALAR_FIELD_TYPES RecordId, std::uint16_t, std::int32_t, float, double, Angle
#define AFW_TABLE_SCALAR_FIELD_TYPE_N 7
#define AFW_TABLE_SCALAR_FIELD_TYPES RecordId, std::uint16_t, std::int32_t, float, double, Angle, std::uint8_t
#define AFW_TABLE_SCALAR_FIELD_TYPE_TUPLE BOOST_PP_LPAREN() AFW_TABLE_SCALAR_FIELD_TYPES BOOST_PP_RPAREN()

// Arrays types: the types we allow for Array fields.
#define AFW_TABLE_ARRAY_FIELD_TYPE_N 4
#define AFW_TABLE_ARRAY_FIELD_TYPES std::uint16_t, int, float, double
#define AFW_TABLE_ARRAY_FIELD_TYPE_N 5
#define AFW_TABLE_ARRAY_FIELD_TYPES std::uint16_t, int, float, double, std::uint8_t
#define AFW_TABLE_ARRAY_FIELD_TYPE_TUPLE BOOST_PP_LPAREN() AFW_TABLE_ARRAY_FIELD_TYPES BOOST_PP_RPAREN()

// Field types: all the types we allow for fields.
#define AFW_TABLE_FIELD_TYPE_N 12
#define AFW_TABLE_FIELD_TYPE_N 14
#define AFW_TABLE_FIELD_TYPES \
AFW_TABLE_SCALAR_FIELD_TYPES, Flag, std::string, Array<std::uint16_t>, Array<int>, Array<float>, \
Copy link

Choose a reason for hiding this comment

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

Unrelated, but is there any reason why we use Array<int> while the scalar is specifically qualified as std::int32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. @TallJimbo?

Copy link
Member

Choose a reason for hiding this comment

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

std::int32_t would be better, but changing it would require concerted changes across a lot of the codebase. Should probably be spun off into another ticket.

Array<double>
Array<double>, Array<std::uint8_t>

#define AFW_TABLE_FIELD_TYPE_TUPLE BOOST_PP_LPAREN() AFW_TABLE_FIELD_TYPES BOOST_PP_RPAREN()

Expand Down
2 changes: 2 additions & 0 deletions python/lsst/afw/table/base/base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,13 @@ PyBaseRecord declareBaseRecord(py::module &mod) {
declareBaseRecordOverloads<double>(cls, "D");
declareBaseRecordOverloads<float>(cls, "F");
declareBaseRecordOverloads<lsst::afw::table::Flag>(cls, "Flag");
declareBaseRecordOverloads<std::uint8_t>(cls, "B");
Copy link

Choose a reason for hiding this comment

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

I really think we should add some kind of type trait at some point to standardize the suffixes. Such that we can say lsst::utils::default_suffix<std::uint8_t>::value and always get the same one.
Maybe not now, since it leads to the slightly tricky problem of compile-time string concatenation, but perhaps file a new ticket for it? If you agree that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this code would benefit from a cleanup pass. But I'm not comfortable enough with type traits to have an opinion on this exact issue.

Copy link
Member

Choose a reason for hiding this comment

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

This is another issue that is worth doing as a separate ticket.

declareBaseRecordOverloads<std::uint16_t>(cls, "U");
declareBaseRecordOverloads<std::int32_t>(cls, "I");
declareBaseRecordOverloads<std::int64_t>(cls, "L");
declareBaseRecordOverloads<std::string>(cls, "String");
declareBaseRecordOverloads<lsst::afw::geom::Angle>(cls, "Angle");
declareBaseRecordArrayOverloads<std::uint8_t>(cls, "ArrayB");
declareBaseRecordArrayOverloads<std::uint16_t>(cls, "ArrayU");
declareBaseRecordArrayOverloads<int>(cls, "ArrayI");
declareBaseRecordArrayOverloads<float>(cls, "ArrayF");
Expand Down
2 changes: 2 additions & 0 deletions python/lsst/afw/table/baseColumnView/baseColumnView.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ static void declareBaseColumnView(py::module &mod) {
// _getBits supports a Python version of getBits that accepts None and field names as keys
cls.def("_getBits", &BaseColumnView::getBits);
cls.def("getAllBits", &BaseColumnView::getAllBits);
declareBaseColumnViewOverloads<std::uint8_t>(cls);
declareBaseColumnViewOverloads<std::uint16_t>(cls);
declareBaseColumnViewOverloads<std::int32_t>(cls);
declareBaseColumnViewOverloads<std::int64_t>(cls);
Expand All @@ -94,6 +95,7 @@ static void declareBaseColumnView(py::module &mod) {
// std::string columns are not supported, because numpy string arrays
// do not have the same memory model as ours.
declareBaseColumnViewOverloads<lsst::afw::geom::Angle>(cls);
declareBaseColumnViewArrayOverloads<std::uint8_t>(cls);
declareBaseColumnViewArrayOverloads<std::uint16_t>(cls);
declareBaseColumnViewArrayOverloads<int>(cls);
declareBaseColumnViewArrayOverloads<float>(cls);
Expand Down
2 changes: 2 additions & 0 deletions python/lsst/afw/table/schema/schema.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,13 +407,15 @@ PYBIND11_PLUGIN(schema) {
mod.attr("_Key") = py::dict();
mod.attr("_SchemaItem") = py::dict();

declareSchemaType<std::uint8_t>(mod);
declareSchemaType<std::uint16_t>(mod);
declareSchemaType<std::int32_t>(mod);
declareSchemaType<std::int64_t>(mod);
declareSchemaType<float>(mod);
declareSchemaType<double>(mod);
declareSchemaType<std::string>(mod);
declareSchemaType<geom::Angle>(mod);
declareSchemaType<Array<std::uint8_t>>(mod);
declareSchemaType<Array<std::uint16_t>>(mod);
declareSchemaType<Array<int>>(mod);
declareSchemaType<Array<float>>(mod);
Expand Down
1 change: 1 addition & 0 deletions python/lsst/afw/table/schema/schemaContinued.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
# Key/Field/SchemaItem types.
_dtypes = {
"String": str,
"B": np.uint8,
"U": np.uint16,
"I": np.int32,
"L": np.int64,
Expand Down
2 changes: 2 additions & 0 deletions python/lsst/afw/table/schemaMapper/schemaMapper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ PYBIND11_PLUGIN(schemaMapper) {
cls.def("addMinimalSchema", &SchemaMapper::addMinimalSchema, "minimal"_a, "doMap"_a = true);
cls.def_static("removeMinimalSchema", &SchemaMapper::removeMinimalSchema);
cls.def_static("join", &SchemaMapper::join, "inputs"_a, "prefixes"_a = std::vector<std::string>());
declareSchemaMapperOverloads<std::uint8_t>(cls, "B");
declareSchemaMapperOverloads<std::uint16_t>(cls, "U");
declareSchemaMapperOverloads<std::int32_t>(cls, "I");
declareSchemaMapperOverloads<std::int64_t>(cls, "L");
Expand All @@ -68,6 +69,7 @@ PYBIND11_PLUGIN(schemaMapper) {
declareSchemaMapperOverloads<std::string>(cls, "String");
declareSchemaMapperOverloads<lsst::afw::geom::Angle>(cls, "Angle");
declareSchemaMapperOverloads<lsst::afw::table::Flag>(cls, "Flag");
declareSchemaMapperOverloads<lsst::afw::table::Array<std::uint8_t>>(cls, "ArrayB");
declareSchemaMapperOverloads<lsst::afw::table::Array<std::uint16_t>>(cls, "ArrayU");
declareSchemaMapperOverloads<lsst::afw::table::Array<int>>(cls, "ArrayI");
declareSchemaMapperOverloads<lsst::afw::table::Array<float>>(cls, "ArrayF");
Expand Down
13 changes: 13 additions & 0 deletions src/fits.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ struct FitsType<char> {
static int const CONSTANT = TSTRING;
};
template <>
struct FitsType<signed char> {
static int const CONSTANT = TSBYTE;
};
template <>
struct FitsType<unsigned char> {
static int const CONSTANT = TBYTE;
};
Copy link

Choose a reason for hiding this comment

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

Any reason we don't specifically also specialize for std::uint8_t? I know it maps on most platforms, but it doesn't have to.

Copy link
Contributor Author

@r-owen r-owen Oct 2, 2017 •
edited

Choose a reason for hiding this comment

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

I'm not sure. I am copying existing code which may be based on this entry in the CFITSIO documentation: https://heasarc.gsfc.nasa.gov/docs/software/fitsio/quick/node9.html. It is true that the entry refers to images (it was the only description of 'TSBYTE' I found).

(begin quote)
The datatype parameter specifies the datatype of the C array in the program, which need not be the same as the datatype of the FITS image itself. If the datatypes differ then CFITSIO will convert the data as it is read or written. The following symbolic constants are allowed for the value of datatype:
TBYTE unsigned char
TSBYTE signed char
TSHORT signed short
TUSHORT unsigned short
TINT signed int
TUINT unsigned int
TLONG signed long
TLONGLONG signed 8-byte integer
TULONG unsigned long
TFLOAT float
TDOUBLE double
(end quote)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, as @r-owen said, this is limited by what cfitsio supports.

Expand Down Expand Up @@ -690,6 +694,15 @@ void writeKeyFromProperty(Fits &fits, daf::base::PropertySet const &metadata, st
} else {
writeKeyImpl(fits, key.c_str(), metadata.get<bool>(key), comment);
}
} else if (valueType == typeid(std::uint8_t)) {
if (metadata.isArray(key)) {
std::vector<std::uint8_t> tmp = metadata.getArray<std::uint8_t>(key);
for (std::size_t i = 0; i != tmp.size(); ++i) {
writeKeyImpl(fits, key.c_str(), tmp[i], comment);
}
} else {
writeKeyImpl(fits, key.c_str(), metadata.get<std::uint8_t>(key), comment);
}
} else if (valueType == typeid(int)) {
if (metadata.isArray(key)) {
std::vector<int> tmp = metadata.getArray<int>(key);
Expand Down
4 changes: 4 additions & 0 deletions src/table/FieldBase.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ namespace {
template <typename T>
struct TypeTraits;

template <>
struct TypeTraits<std::uint8_t> {
static char const *getName() { return "B"; }
Copy link

Choose a reason for hiding this comment

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

Ok, so we have one. Then why don't we use it everywhere? Preferably pulling it up to utils?
In that case I would still modify it slightly such that it can be used in constexpr contexts (e.g. one can use it in a switch or something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These characters are FITS table characters. I'm not sure I'd want to use it anywhere else.

I don't understand your suggested modification but respectfully suggest that any significant refactoring should go into a new ticket.

};
template <>
struct TypeTraits<std::uint16_t> {
static char const *getName() { return "U"; }
Expand Down
12 changes: 12 additions & 0 deletions src/table/io/FitsSchemaInputMapper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,18 @@ std::unique_ptr<FitsColumnReader> makeColumnReader(Schema &schema, FitsSchemaIte
}
// switch code over FITS codes that correspond to different element types
switch (code) {
case 'B': // 8-bit unsigned integers -- can only be scalars or Arrays
if (size == 1) {
if (item.tccls == "Array") {
return StandardReader<Array<std::uint8_t>>::make(schema, item, size);
}
return StandardReader<std::uint8_t>::make(schema, item);
}
if (size == 0) {
return VariableLengthArrayReader<std::uint8_t>::make(schema, item);
}
return StandardReader<Array<std::uint8_t>>::make(schema, item, size);

case 'I': // 16-bit integers - can only be scalars or Arrays (we assume they're unsigned, since
// that's all we ever write, and CFITSIO will complain later if they aren't)
if (size == 1) {
Expand Down