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

r-owen · 2017-09-23T15:42:12Z

No description provided.

Rename some items to make it easier to see what's going on and test additional data types. Add a few obvious additional tests. Modernize a few tests.

pschella · 2017-09-30T15:05:48Z

include/lsst/afw/table/types.h

 #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>, \


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

I have no idea. @TallJimbo?

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.

pschella · 2017-09-30T15:07:15Z

python/lsst/afw/table/base/base.cc

@@ -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");


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.

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.

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

pschella · 2017-09-30T15:12:45Z

src/fits.cc

+struct FitsType<signed char> {
+    static int const CONSTANT = TSBYTE;
+};
+template <>
 struct FitsType<unsigned char> {
    static int const CONSTANT = TBYTE;
 };


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.

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)

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

pschella · 2017-09-30T15:19:19Z

src/table/FieldBase.cc

@@ -20,6 +20,10 @@ template <typename T>
 struct TypeTraits;

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


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

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.

pschella · 2017-09-30T15:31:46Z

include/lsst/afw/formatters/Utils.h

+/**
+ * Decode a std::string from a vector of uint8 returned by stringToBytes
+ */
+std::string bytesToString(ndarray::Array<std::uint8_t const, 1, 1> bytes);


Why pass by value?

I'll change it. It shouldn't matter because ndarray uses a pointer to the data internally, but it will be more obviously safe if I change it.

pschella · 2017-09-30T15:36:39Z

src/formatters/Utils.cc

@@ -286,6 +286,24 @@ std::string formatFitsProperties(daf::base::PropertyList const& prop,
 int countFitsHeaderCards(lsst::daf::base::PropertySet const& prop) {
    return prop.paramNames(false).size();
 }
+
+ndarray::Array<std::uint8_t, 1, 1> stringToBytes(std::string const& str) {
+    auto nbytes = str.size() * sizeof(char) / sizeof(std::uint8_t);


sizeof(typename std::string::value_type)? Or do we trust the user to update this if they switch the input to some other string type?

std::string::data is documented to return const char * so I think the current code is safest.

pschella · 2017-09-30T15:48:58Z

src/formatters/Utils.cc

+
+ndarray::Array<std::uint8_t, 1, 1> stringToBytes(std::string const& str) {
+    auto nbytes = str.size() * sizeof(char) / sizeof(std::uint8_t);
+    std::uint8_t const* byteCArr = reinterpret_cast<std::uint8_t const*>(str.data());


So we don't do any formal encoding? Hmm, I'm very worried that this will create problems when the data is read on a different platform then the one it was written.

Discussed on the JIRA ticket with the resolution that we will file another ticket to look at this and use this code for now.

from using a variable-length string field to a variable-length array of uint8 encoding the transform's string representation. Update the test_transformBoundedField to torture-test that on a transform with a long string representation.

r-owen force-pushed the tickets/DM-11957 branch from bf7a6d0 to 2a6107e Compare September 25, 2017 19:56

r-owen added 2 commits September 29, 2017 17:14

Minor enhancements to test_simpleTable.py

8de7d66

Rename some items to make it easier to see what's going on and test additional data types. Add a few obvious additional tests. Modernize a few tests.

Add uint8 scalar and array field types

c20f022

r-owen force-pushed the tickets/DM-11957 branch from 2a6107e to 490acaa Compare September 30, 2017 00:15

pschella reviewed Sep 30, 2017

View reviewed changes

r-owen added 5 commits October 3, 2017 12:59

Add stringToBytes and bytesToString

abc3b23

Remove an unused library module

3d2d45a

Change TransformBoundedField table persistence

43af436

from using a variable-length string field to a variable-length array of uint8 encoding the transform's string representation. Update the test_transformBoundedField to torture-test that on a transform with a long string representation.

Fix doxygen errors in ImageBase and MaskedImage

adf97df

Fix doxygen warning in fitsCompression.cc

6ecb45e

r-owen force-pushed the tickets/DM-11957 branch from e2ee293 to 6ecb45e Compare October 3, 2017 20:03

r-owen merged commit 6ecb45e into master Oct 3, 2017

ktlim deleted the tickets/DM-11957 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

r-owen commented Sep 23, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017

TallJimbo Oct 3, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017

TallJimbo Oct 3, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017 •

edited

TallJimbo Oct 3, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017

pschella Sep 30, 2017

r-owen Oct 2, 2017

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

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

Conversation

r-owen commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 2, 2017 •

edited