From b73402e21a50c68706d0058b5c9dee088150e261 Mon Sep 17 00:00:00 2001 From: Jim Bosch Date: Sat, 6 Feb 2021 11:48:05 -0500 Subject: [PATCH 1/3] Move covariance-indexing funcs to the only place they're used. The compound field types that once used these are gone, so the only place we need them is in the backwards-compatibility reading code. --- include/lsst/afw/table/FieldBase.h | 16 ---------------- src/table/io/FitsSchemaInputMapper.cc | 20 +++++++++++++++++--- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/include/lsst/afw/table/FieldBase.h b/include/lsst/afw/table/FieldBase.h index 5e43e753f4..227ac1cc3f 100644 --- a/include/lsst/afw/table/FieldBase.h +++ b/include/lsst/afw/table/FieldBase.h @@ -20,22 +20,6 @@ namespace lsst { namespace afw { namespace table { -namespace detail { - -class TableImpl; - -/** - * Defines the ordering of packed covariance matrices. - * - * This storage is equivalent to LAPACK 'UPLO=U'. - */ -inline int indexCovariance(int i, int j) { return (i < j) ? (i + j * (j + 1) / 2) : (j + i * (i + 1) / 2); } - -/// Defines the packed size of a covariance matrices. -inline int computeCovariancePackedSize(int size) { return size * (size + 1) / 2; } - -} // namespace detail - /** * Field base class default implementation (used for numeric scalars and lsst::geom::Angle). */ diff --git a/src/table/io/FitsSchemaInputMapper.cc b/src/table/io/FitsSchemaInputMapper.cc index 931ae36ae1..44a9cd636d 100644 --- a/src/table/io/FitsSchemaInputMapper.cc +++ b/src/table/io/FitsSchemaInputMapper.cc @@ -595,6 +595,20 @@ class CovarianceConversionReader : public FitsColumnReader { return oldUnits; } + /** + * Defines the ordering of packed covariance matrices. + * + * This storage is equivalent to LAPACK 'UPLO=U'. + */ + static int indexCovariance(int i, int j) { + return (i < j) ? (i + j * (j + 1) / 2) : (j + i * (i + 1) / 2); + } + + /// Defines the packed size of a covariance matrices. + static int computeCovariancePackedSize(int size) { + return size * (size + 1) / 2; + } + static std::unique_ptr make(Schema &schema, FitsSchemaItem const &item, std::vector const &names) { return std::unique_ptr(new CovarianceConversionReader(schema, item, names)); @@ -605,14 +619,14 @@ class CovarianceConversionReader : public FitsColumnReader { : _column(item.column), _size(names.size()), _key(CovarianceMatrixKey::addFields(schema, item.ttype, names, guessUnits(item.tunit))), - _buffer(new T[detail::computeCovariancePackedSize(names.size())]) {} + _buffer(new T[computeCovariancePackedSize(names.size())]) {} void readCell(BaseRecord &record, std::size_t row, afw::fits::Fits &fits, std::shared_ptr const &archive) const override { - fits.readTableArray(row, _column, detail::computeCovariancePackedSize(_size), _buffer.get()); + fits.readTableArray(row, _column, computeCovariancePackedSize(_size), _buffer.get()); for (int i = 0; i < _size; ++i) { for (int j = i; j < _size; ++j) { - _key.setElement(record, i, j, _buffer[detail::indexCovariance(i, j)]); + _key.setElement(record, i, j, _buffer[indexCovariance(i, j)]); } } } From b53abcb130c90edee210c94f1efcb8d48d858290 Mon Sep 17 00:00:00 2001 From: Jim Bosch Date: Sat, 6 Feb 2021 13:00:54 -0500 Subject: [PATCH 2/3] Restructure headers and includes in afw.table. This commit guards against one-definition rule violations involving explicit specializations of the Key, KeyBase, and FieldBase classes by moving those specializations next to the default definitions of those templates and generally making includes more "self-sufficient": headers should now (at least mostly) include the headers for all classes they use directly, and should not rely on other headers being included for them in any source files (but that is always difficult to check). In a few cases I do assume that some headers (e.g. Schema.h) can be relied upon to include others (Key.h, Field.h) in higher-level code. This also removes a few includes that were not being used in those headers at all. That could cause downstream compile failures if source files or other headers were (incorrectly) relying on those incorrect includes, but such breakage should be easy to fix and is worth fixing anyway. --- include/lsst/afw/table/AliasMap.h | 2 + include/lsst/afw/table/Field.h | 1 + include/lsst/afw/table/FieldBase.h | 51 ++++- include/lsst/afw/table/Flag.h | 193 +----------------- include/lsst/afw/table/Key.h | 114 ++++++++++- include/lsst/afw/table/KeyBase.h | 37 +++- include/lsst/afw/table/Schema.h | 1 - include/lsst/afw/table/SchemaMapper.h | 3 + include/lsst/afw/table/aggregates.h | 7 +- include/lsst/afw/table/arrays.h | 4 + include/lsst/afw/table/detail/Access.h | 7 +- include/lsst/afw/table/detail/SchemaImpl.h | 5 +- .../lsst/afw/table/detail/SchemaMapperImpl.h | 2 + include/lsst/afw/table/fwd.h | 7 +- include/lsst/afw/table/misc.h | 22 +- include/lsst/afw/table/types.h | 10 +- src/table/FieldBase.cc | 2 +- src/table/aggregates.cc | 2 +- src/table/io/FitsSchemaInputMapper.cc | 1 + 19 files changed, 249 insertions(+), 222 deletions(-) diff --git a/include/lsst/afw/table/AliasMap.h b/include/lsst/afw/table/AliasMap.h index 5b835a874a..75131a8313 100644 --- a/include/lsst/afw/table/AliasMap.h +++ b/include/lsst/afw/table/AliasMap.h @@ -5,6 +5,8 @@ #include #include +#include "lsst/afw/table/fwd.h" + namespace lsst { namespace afw { namespace table { diff --git a/include/lsst/afw/table/Field.h b/include/lsst/afw/table/Field.h index 05fd20a545..402586602b 100644 --- a/include/lsst/afw/table/Field.h +++ b/include/lsst/afw/table/Field.h @@ -2,6 +2,7 @@ #ifndef AFW_TABLE_Field_h_INCLUDED #define AFW_TABLE_Field_h_INCLUDED +#include #include #include "lsst/afw/table/FieldBase.h" diff --git a/include/lsst/afw/table/FieldBase.h b/include/lsst/afw/table/FieldBase.h index 227ac1cc3f..d9df577a5f 100644 --- a/include/lsst/afw/table/FieldBase.h +++ b/include/lsst/afw/table/FieldBase.h @@ -2,19 +2,13 @@ #ifndef AFW_TABLE_FieldBase_h_INCLUDED #define AFW_TABLE_FieldBase_h_INCLUDED -#include +#include #include -#include "boost/mpl/vector.hpp" -#include "boost/preprocessor/punctuation/paren.hpp" -#include "Eigen/Core" +#include "ndarray.h" -#include "lsst/base.h" #include "lsst/pex/exceptions.h" -#include "ndarray.h" #include "lsst/afw/table/misc.h" -#include "lsst/afw/table/KeyBase.h" -#include "lsst/afw/table/types.h" namespace lsst { namespace afw { @@ -291,6 +285,47 @@ struct FieldBase { private: int _size; }; + +/** + * Specialization for Flag fields. + * + * Flag fields are handled specially in many places, because their keys have both an offset into an + * integer element and the bit in that element; while other fields have one or more elements per field, + * Flags have multiple fields per element. This means we can't put all the custom code for Flag in + * FieldBase, and because Flags have an explicit Key specialization, we put the record access + * implementation in Key. + */ +template <> +struct FieldBase { + typedef bool Value; ///< the type returned by BaseRecord::get + typedef std::int64_t Element; ///< the actual storage type (shared by multiple flag fields) + + /// Return the number of subfield elements (always one for scalars). + int getElementCount() const { return 1; } + + /// Return a string description of the field type. + static std::string getTypeString() { return "Flag"; } + + // Only the first of these constructors is valid for this specializations, but + // it's convenient to be able to instantiate both, since the other is used + // by other specializations. + FieldBase() = default; + FieldBase(int) { + throw LSST_EXCEPT(lsst::pex::exceptions::LogicError, + "Constructor disabled (this Field type is not sized)."); + } + + FieldBase(FieldBase const &) = default; + FieldBase(FieldBase &&) = default; + FieldBase &operator=(FieldBase const &) = default; + FieldBase &operator=(FieldBase &&) = default; + ~FieldBase() = default; + +protected: + /// Defines how fields are printed. + void stream(std::ostream &os) const {} +}; + } // namespace table } // namespace afw } // namespace lsst diff --git a/include/lsst/afw/table/Flag.h b/include/lsst/afw/table/Flag.h index ddb8895321..344c8c29f0 100644 --- a/include/lsst/afw/table/Flag.h +++ b/include/lsst/afw/table/Flag.h @@ -2,195 +2,14 @@ #ifndef LSST_AFW_TABLE_Flag_h_INCLUDED #define LSST_AFW_TABLE_Flag_h_INCLUDED -#include +// This is a backwards-compatibility header; the template specializations for +// flag have now been included in the same headers as the default definitions +// of those templates (included below). This guards against implicit +// instantiation of the default templates for Flag, which would be a violation +// of the One Definition Rule. -#include "lsst/utils/hashCombine.h" - -#include "lsst/afw/table/misc.h" #include "lsst/afw/table/FieldBase.h" #include "lsst/afw/table/KeyBase.h" - -namespace lsst { -namespace afw { -namespace table { - -namespace detail { - -class Access; - -} // namespace detail - -/** - * Specialization for Flag fields. - * - * Flag fields are handled specially in many places, because their keys have both an offset into an - * integer element and the bit in that element; while other fields have one or more elements per field, - * Flags have multiple fields per element. This means we can't put all the custom code for Flag in - * FieldBase, and because Flags have an explicit Key specialization, we put the record access - * implementation in Key. - */ -template <> -struct FieldBase { - typedef bool Value; ///< the type returned by BaseRecord::get - typedef std::int64_t Element; ///< the actual storage type (shared by multiple flag fields) - - /// Return the number of subfield elements (always one for scalars). - int getElementCount() const { return 1; } - - /// Return a string description of the field type. - static std::string getTypeString() { return "Flag"; } - - // Only the first of these constructors is valid for this specializations, but - // it's convenient to be able to instantiate both, since the other is used - // by other specializations. - FieldBase() = default; - FieldBase(int) { - throw LSST_EXCEPT(lsst::pex::exceptions::LogicError, - "Constructor disabled (this Field type is not sized)."); - } - - FieldBase(FieldBase const &) = default; - FieldBase(FieldBase &&) = default; - FieldBase &operator=(FieldBase const &) = default; - FieldBase &operator=(FieldBase &&) = default; - ~FieldBase() = default; - -protected: - /// Defines how fields are printed. - void stream(std::ostream &os) const {} -}; - -/** - * A base class for Key that allows the underlying storage field to be extracted. - */ -template <> -class KeyBase { -public: - static bool const HAS_NAMED_SUBFIELDS = false; - - /// Return a key corresponding to the integer element where this field's bit is packed. - Key::Element> getStorage() const; - - KeyBase() = default; - KeyBase(KeyBase const &) = default; - KeyBase(KeyBase &&) = default; - KeyBase &operator=(KeyBase const &) = default; - KeyBase &operator=(KeyBase &&) = default; - ~KeyBase() = default; -}; - -/** - * Key specialization for Flag. - * - * Flag fields are special; their keys need to contain not only the offset to the - * integer element they share with other Flag fields, but also their position - * in that shared field. - * - * Flag fields operate mostly like a bool field, but they do not support reference - * access, and internally they are packed into an integer shared by multiple fields - * so the marginal cost of each Flag field is only one bit. - */ -template <> -class Key : public KeyBase, public FieldBase { -public: - //@{ - /** - * Equality comparison. - * - * Two keys with different types are never equal. Keys with the same type - * are equal if they point to the same location in a table, regardless of - * what Schema they were constructed from (for instance, if a field has a - * different name in one Schema than another, but is otherwise the same, - * the two keys will be equal). - */ - template - bool operator==(Key const &other) const { - return false; - } - template - bool operator!=(Key const &other) const { - return true; - } - - bool operator==(Key const &other) const { return _offset == other._offset && _bit == other._bit; } - bool operator!=(Key const &other) const { return !this->operator==(other); } - //@} - - /// Return a hash of this object. - std::size_t hash_value() const noexcept { - // Completely arbitrary seed - return utils::hashCombine(17, _offset, _bit); - } - - /// Return the offset in bytes of the integer element that holds this field's bit. - int getOffset() const { return _offset; } - - /// The index of this field's bit within the integer it shares with other Flag fields. - int getBit() const { return _bit; } - - /** - * Return true if the key was initialized to valid offset. - * - * This does not guarantee that a key is valid with any particular schema, or even - * that any schemas still exist in which this key is valid. - * - * A key that is default constructed will always be invalid. - */ - bool isValid() const { return _offset >= 0; } - - /** - * Default construct a field. - * - * The new field will be invalid until a valid Key is assigned to it. - */ - Key() : FieldBase(), _offset(-1), _bit(0) {} - - Key(Key const &) = default; - Key(Key &&) = default; - Key &operator=(Key const &) = default; - Key &operator=(Key &&) = default; - ~Key() = default; - - /// Stringification. - inline friend std::ostream &operator<<(std::ostream &os, Key const &key) { - return os << "Key['" << Key::getTypeString() << "'](offset=" << key.getOffset() - << ", bit=" << key.getBit() << ")"; - } - -private: - friend class detail::Access; - friend class BaseRecord; - - /// Used to implement BaseRecord::get. - Value getValue(Element const *p, ndarray::Manager::Ptr const &) const { - return (*p) & (Element(1) << _bit); - } - - /// Used to implement BaseRecord::set. - void setValue(Element *p, ndarray::Manager::Ptr const &, Value v) const { - if (v) { - *p |= (Element(1) << _bit); - } else { - *p &= ~(Element(1) << _bit); - } - } - - explicit Key(int offset, int bit) : _offset(offset), _bit(bit) {} - - int _offset; - int _bit; -}; -} // namespace table -} // namespace afw -} // namespace lsst - -namespace std { -template <> -struct hash> { - using argument_type = lsst::afw::table::Key; - using result_type = size_t; - size_t operator()(argument_type const &obj) const noexcept { return obj.hash_value(); } -}; -} // namespace std +#include "lsst/afw/table/Key.h" #endif // !LSST_AFW_TABLE_Flag_h_INCLUDED diff --git a/include/lsst/afw/table/Key.h b/include/lsst/afw/table/Key.h index bdabaee410..b96d5be4ab 100644 --- a/include/lsst/afw/table/Key.h +++ b/include/lsst/afw/table/Key.h @@ -2,10 +2,11 @@ #ifndef AFW_TABLE_Key_h_INCLUDED #define AFW_TABLE_Key_h_INCLUDED +#include + #include "lsst/utils/hashCombine.h" #include "lsst/afw/table/FieldBase.h" -#include "lsst/afw/table/Flag.h" #include "lsst/afw/table/KeyBase.h" namespace lsst { @@ -124,6 +125,109 @@ class Key : public KeyBase, public FieldBase { int _offset; }; + +/** + * Key specialization for Flag. + * + * Flag fields are special; their keys need to contain not only the offset to the + * integer element they share with other Flag fields, but also their position + * in that shared field. + * + * Flag fields operate mostly like a bool field, but they do not support reference + * access, and internally they are packed into an integer shared by multiple fields + * so the marginal cost of each Flag field is only one bit. + */ +template <> +class Key : public KeyBase, public FieldBase { +public: + //@{ + /** + * Equality comparison. + * + * Two keys with different types are never equal. Keys with the same type + * are equal if they point to the same location in a table, regardless of + * what Schema they were constructed from (for instance, if a field has a + * different name in one Schema than another, but is otherwise the same, + * the two keys will be equal). + */ + template + bool operator==(Key const &other) const { + return false; + } + template + bool operator!=(Key const &other) const { + return true; + } + + bool operator==(Key const &other) const { return _offset == other._offset && _bit == other._bit; } + bool operator!=(Key const &other) const { return !this->operator==(other); } + //@} + + /// Return a hash of this object. + std::size_t hash_value() const noexcept { + // Completely arbitrary seed + return utils::hashCombine(17, _offset, _bit); + } + + /// Return the offset in bytes of the integer element that holds this field's bit. + int getOffset() const { return _offset; } + + /// The index of this field's bit within the integer it shares with other Flag fields. + int getBit() const { return _bit; } + + /** + * Return true if the key was initialized to valid offset. + * + * This does not guarantee that a key is valid with any particular schema, or even + * that any schemas still exist in which this key is valid. + * + * A key that is default constructed will always be invalid. + */ + bool isValid() const { return _offset >= 0; } + + /** + * Default construct a field. + * + * The new field will be invalid until a valid Key is assigned to it. + */ + Key() : FieldBase(), _offset(-1), _bit(0) {} + + Key(Key const &) = default; + Key(Key &&) = default; + Key &operator=(Key const &) = default; + Key &operator=(Key &&) = default; + ~Key() = default; + + /// Stringification. + inline friend std::ostream &operator<<(std::ostream &os, Key const &key) { + return os << "Key['" << Key::getTypeString() << "'](offset=" << key.getOffset() + << ", bit=" << key.getBit() << ")"; + } + +private: + friend class detail::Access; + friend class BaseRecord; + + /// Used to implement BaseRecord::get. + Value getValue(Element const *p, ndarray::Manager::Ptr const &) const { + return (*p) & (Element(1) << _bit); + } + + /// Used to implement BaseRecord::set. + void setValue(Element *p, ndarray::Manager::Ptr const &, Value v) const { + if (v) { + *p |= (Element(1) << _bit); + } else { + *p &= ~(Element(1) << _bit); + } + } + + explicit Key(int offset, int bit) : _offset(offset), _bit(bit) {} + + int _offset; + int _bit; +}; + } // namespace table } // namespace afw } // namespace lsst @@ -135,6 +239,14 @@ struct hash> { using result_type = size_t; size_t operator()(argument_type const& obj) const noexcept { return obj.hash_value(); } }; + +template <> +struct hash> { + using argument_type = lsst::afw::table::Key; + using result_type = size_t; + size_t operator()(argument_type const &obj) const noexcept { return obj.hash_value(); } +}; + } // namespace std #endif // !AFW_TABLE_Key_h_INCLUDED diff --git a/include/lsst/afw/table/KeyBase.h b/include/lsst/afw/table/KeyBase.h index 3a1ade65ab..25acba7c72 100644 --- a/include/lsst/afw/table/KeyBase.h +++ b/include/lsst/afw/table/KeyBase.h @@ -5,6 +5,7 @@ #include #include "lsst/afw/table/misc.h" +#include "lsst/afw/table/FieldBase.h" namespace lsst { namespace afw { @@ -12,14 +13,18 @@ namespace table { class BaseRecord; -template -class Key; - /// A base class for Key that allows subfield keys to be extracted for some field types. template class KeyBase { public: static bool const HAS_NAMED_SUBFIELDS = false; + + KeyBase() = default; + KeyBase(KeyBase const &) = default; + KeyBase(KeyBase &&) = default; + KeyBase &operator=(KeyBase const &) = default; + KeyBase &operator=(KeyBase &&) = default; + ~KeyBase() = default; }; /// KeyBase specialization for Arrays. @@ -28,6 +33,13 @@ class KeyBase > { public: static bool const HAS_NAMED_SUBFIELDS = false; + KeyBase() = default; + KeyBase(KeyBase const &) = default; + KeyBase(KeyBase &&) = default; + KeyBase &operator=(KeyBase const &) = default; + KeyBase &operator=(KeyBase &&) = default; + ~KeyBase() = default; + std::vector extractVector(BaseRecord const& record) const; void assignVector(BaseRecord& record, std::vector const& values) const; @@ -36,6 +48,25 @@ class KeyBase > { Key > slice(int begin, int end) const; ///< Return a key for a range of elements }; + +/// KeyBase specialization for Flags. +template <> +class KeyBase { +public: + static bool const HAS_NAMED_SUBFIELDS = false; + + KeyBase() = default; + KeyBase(KeyBase const &) = default; + KeyBase(KeyBase &&) = default; + KeyBase &operator=(KeyBase const &) = default; + KeyBase &operator=(KeyBase &&) = default; + ~KeyBase() = default; + + /// Return a key corresponding to the integer element where this field's bit is packed. + Key::Element> getStorage() const; + +}; + } // namespace table } // namespace afw } // namespace lsst diff --git a/include/lsst/afw/table/Schema.h b/include/lsst/afw/table/Schema.h index c03b9e9bf8..adf86c7c01 100644 --- a/include/lsst/afw/table/Schema.h +++ b/include/lsst/afw/table/Schema.h @@ -12,7 +12,6 @@ #include "lsst/afw/table/Key.h" #include "lsst/afw/table/Field.h" #include "lsst/afw/table/detail/SchemaImpl.h" -#include "lsst/afw/table/Flag.h" #include "lsst/afw/table/AliasMap.h" namespace lsst { diff --git a/include/lsst/afw/table/SchemaMapper.h b/include/lsst/afw/table/SchemaMapper.h index 742dc24c32..4da45db6ec 100644 --- a/include/lsst/afw/table/SchemaMapper.h +++ b/include/lsst/afw/table/SchemaMapper.h @@ -4,6 +4,9 @@ #include +#include "lsst/afw/table/Key.h" +#include "lsst/afw/table/Field.h" +#include "lsst/afw/table/Schema.h" #include "lsst/afw/table/detail/SchemaMapperImpl.h" namespace lsst { diff --git a/include/lsst/afw/table/aggregates.h b/include/lsst/afw/table/aggregates.h index e9f4c282c2..9c9329d82b 100644 --- a/include/lsst/afw/table/aggregates.h +++ b/include/lsst/afw/table/aggregates.h @@ -23,11 +23,15 @@ #ifndef AFW_TABLE_aggregates_h_INCLUDED #define AFW_TABLE_aggregates_h_INCLUDED +#include +#include +#include "Eigen/Core" + #include "lsst/utils/hashCombine.h" +#include "lsst/geom.h" #include "lsst/afw/table/FunctorKey.h" #include "lsst/afw/table/Schema.h" -#include "lsst/geom.h" namespace lsst { namespace afw { @@ -36,6 +40,7 @@ namespace geom { namespace ellipses { class Quadrupole; +class Ellipse; } // namespace ellipses } // namespace geom diff --git a/include/lsst/afw/table/arrays.h b/include/lsst/afw/table/arrays.h index 2f8fccb77d..9127af0155 100644 --- a/include/lsst/afw/table/arrays.h +++ b/include/lsst/afw/table/arrays.h @@ -23,6 +23,10 @@ #ifndef AFW_TABLE_arrays_h_INCLUDED #define AFW_TABLE_arrays_h_INCLUDED +#include +#include +#include "ndarray.h" + #include "lsst/utils/hashCombine.h" #include "lsst/afw/table/FunctorKey.h" diff --git a/include/lsst/afw/table/detail/Access.h b/include/lsst/afw/table/detail/Access.h index b6ddaca7f3..c0dbbe4ac8 100644 --- a/include/lsst/afw/table/detail/Access.h +++ b/include/lsst/afw/table/detail/Access.h @@ -2,12 +2,13 @@ #ifndef AFW_TABLE_DETAIL_Access_h_INCLUDED #define AFW_TABLE_DETAIL_Access_h_INCLUDED -#include +#include #include "ndarray/Manager.h" -#include "lsst/afw/table/FieldBase.h" +#include "lsst/afw/table/misc.h" +#include "lsst/afw/table/KeyBase.h" +#include "lsst/afw/table/Key.h" #include "lsst/afw/table/Schema.h" -#include "lsst/afw/table/detail/SchemaImpl.h" namespace lsst { namespace afw { diff --git a/include/lsst/afw/table/detail/SchemaImpl.h b/include/lsst/afw/table/detail/SchemaImpl.h index 160c3b2286..cdccf26f08 100644 --- a/include/lsst/afw/table/detail/SchemaImpl.h +++ b/include/lsst/afw/table/detail/SchemaImpl.h @@ -5,11 +5,14 @@ #include #include #include -#include #include "boost/variant.hpp" #include "boost/mpl/transform.hpp" +#include "lsst/afw/table/Key.h" +#include "lsst/afw/table/Field.h" +#include "lsst/afw/table/types.h" + namespace lsst { namespace afw { namespace table { diff --git a/include/lsst/afw/table/detail/SchemaMapperImpl.h b/include/lsst/afw/table/detail/SchemaMapperImpl.h index 1bed0b06d9..fed6f4ca9e 100644 --- a/include/lsst/afw/table/detail/SchemaMapperImpl.h +++ b/include/lsst/afw/table/detail/SchemaMapperImpl.h @@ -8,6 +8,8 @@ #include "boost/variant.hpp" #include "boost/mpl/transform.hpp" +#include "lsst/afw/table/Key.h" +#include "lsst/afw/table/types.h" #include "lsst/afw/table/Schema.h" namespace lsst { diff --git a/include/lsst/afw/table/fwd.h b/include/lsst/afw/table/fwd.h index 45c16e77bc..bacf427a98 100644 --- a/include/lsst/afw/table/fwd.h +++ b/include/lsst/afw/table/fwd.h @@ -41,10 +41,9 @@ namespace lsst { namespace afw { namespace table { -template -class Key; -template -struct Field; +// Some schema primitives (Key, Field, KeyBase, FieldBase) are forward-declared +// in misc.h, along with their explicit specializations. + template struct SchemaItem; class Schema; diff --git a/include/lsst/afw/table/misc.h b/include/lsst/afw/table/misc.h index bb79096141..567a0bc82f 100644 --- a/include/lsst/afw/table/misc.h +++ b/include/lsst/afw/table/misc.h @@ -3,8 +3,7 @@ #define AFW_TABLE_misc_h_INCLUDED #include - -#include "boost/mpl/if.hpp" +#include #include "lsst/geom/Angle.h" #include "lsst/geom/SpherePoint.h" @@ -34,6 +33,25 @@ class Flag; typedef lsst::geom::Angle Angle; typedef lsst::geom::SpherePoint SpherePoint; //@} + + +// Forward-declare schema primitives, and then forward-declare all +// explicit specializations, to guard against code implicitly instantiating +// the default template for any of those. + +template class KeyBase; +template class FieldBase; +template class Key; +template class Field; + +template <> class KeyBase; +template <> class FieldBase; +template <> class Key; + +template class KeyBase>; +template class FieldBase>; +template <> class FieldBase; + } // namespace table } // namespace afw } // namespace lsst diff --git a/include/lsst/afw/table/types.h b/include/lsst/afw/table/types.h index f8e9a4507b..14c18d398c 100644 --- a/include/lsst/afw/table/types.h +++ b/include/lsst/afw/table/types.h @@ -8,16 +8,8 @@ #include "boost/mpl/vector.hpp" #include "boost/preprocessor/punctuation/paren.hpp" -#include "Eigen/Core" - -#include "lsst/base.h" -#include "lsst/pex/exceptions.h" -#include "ndarray.h" -#include "lsst/geom.h" -#include "lsst/afw/geom/ellipses.h" -#include "lsst/afw/coord.h" + #include "lsst/afw/table/misc.h" -#include "lsst/afw/table/KeyBase.h" /* * This file contains macros and MPL vectors that list the types that can be used for fields. diff --git a/src/table/FieldBase.cc b/src/table/FieldBase.cc index 319006465e..7bd7760237 100644 --- a/src/table/FieldBase.cc +++ b/src/table/FieldBase.cc @@ -8,7 +8,7 @@ #include "boost/preprocessor/tuple/to_seq.hpp" #include "lsst/afw/table/FieldBase.h" -#include "lsst/afw/table/Flag.h" +#include "lsst/afw/table/types.h" namespace lsst { namespace afw { diff --git a/src/table/aggregates.cc b/src/table/aggregates.cc index 8af5fdd9c9..082236d676 100644 --- a/src/table/aggregates.cc +++ b/src/table/aggregates.cc @@ -22,7 +22,7 @@ */ #include "lsst/afw/geom/ellipses/Quadrupole.h" -#include "lsst/geom/Box.h" +#include "lsst/afw/geom/ellipses/Ellipse.h" #include "lsst/afw/table/aggregates.h" #include "lsst/afw/table/BaseRecord.h" diff --git a/src/table/io/FitsSchemaInputMapper.cc b/src/table/io/FitsSchemaInputMapper.cc index 44a9cd636d..8577085a01 100644 --- a/src/table/io/FitsSchemaInputMapper.cc +++ b/src/table/io/FitsSchemaInputMapper.cc @@ -17,6 +17,7 @@ #include "lsst/log/Log.h" #include "lsst/geom.h" +#include "lsst/afw/geom/ellipses/Quadrupole.h" #include "lsst/afw/table/io/FitsSchemaInputMapper.h" #include "lsst/afw/table/aggregates.h" From 1d8a8cd7d24ad1bfc115210ae5264a9f4cc387cf Mon Sep 17 00:00:00 2001 From: Jim Bosch Date: Mon, 8 Feb 2021 11:39:33 -0500 Subject: [PATCH 3/3] Remove vestigial subfields concept from afw.table. We no longer have any compound keys - they've been replaced by the FunctorKey concept instead. This lets us simplify a lot of code, especially in Schema.cc. --- include/lsst/afw/table/KeyBase.h | 6 - python/lsst/afw/table/_base.py | 25 +--- python/lsst/afw/table/_schema.cc | 1 - src/table/KeyBase.cc | 8 -- src/table/Schema.cc | 201 ++----------------------------- 5 files changed, 18 insertions(+), 223 deletions(-) diff --git a/include/lsst/afw/table/KeyBase.h b/include/lsst/afw/table/KeyBase.h index 25acba7c72..fac105a03c 100644 --- a/include/lsst/afw/table/KeyBase.h +++ b/include/lsst/afw/table/KeyBase.h @@ -17,8 +17,6 @@ class BaseRecord; template class KeyBase { public: - static bool const HAS_NAMED_SUBFIELDS = false; - KeyBase() = default; KeyBase(KeyBase const &) = default; KeyBase(KeyBase &&) = default; @@ -31,8 +29,6 @@ class KeyBase { template class KeyBase > { public: - static bool const HAS_NAMED_SUBFIELDS = false; - KeyBase() = default; KeyBase(KeyBase const &) = default; KeyBase(KeyBase &&) = default; @@ -53,8 +49,6 @@ class KeyBase > { template <> class KeyBase { public: - static bool const HAS_NAMED_SUBFIELDS = false; - KeyBase() = default; KeyBase(KeyBase const &) = default; KeyBase(KeyBase &&) = default; diff --git a/python/lsst/afw/table/_base.py b/python/lsst/afw/table/_base.py index 248dad909e..edd4f8d5eb 100644 --- a/python/lsst/afw/table/_base.py +++ b/python/lsst/afw/table/_base.py @@ -32,11 +32,11 @@ class BaseRecord: # noqa: F811 def extract(self, *patterns, **kwargs): - """Extract a dictionary of {: } in which the field names - match the given shell-style glob pattern(s). + """Extract a dictionary of {: } in which the field + names match the given shell-style glob pattern(s). - Any number of glob patterns may be passed; the result will be the union of all - the result of each glob considered separately. + Any number of glob patterns may be passed; the result will be the union + of all the result of each glob considered separately. Parameters ---------- @@ -46,11 +46,6 @@ def extract(self, *patterns, **kwargs): to be reused to extract values from multiple records. This keyword is incompatible with any position arguments and the regex, sub, and ordered keyword arguments. - split : `bool` - If `True`, fields with named subfields (e.g. points) will be split - into separate items in the dict; instead of {"point": - lsst.geom.Point2I(2,3)}, for instance, you'd get {"point.x": - 2, "point.y": 3}. Default is `False`. regex : `str` or `re` pattern object A regular expression to be used in addition to any glob patterns passed as positional arguments. Note that this will be compared @@ -64,22 +59,12 @@ def extract(self, *patterns, **kwargs): order of the `Schema`. Default is `False`. """ d = kwargs.pop("items", None) - split = kwargs.pop("split", False) if d is None: d = self.schema.extract(*patterns, **kwargs).copy() elif kwargs: kwargsStr = ", ".join(kwargs.keys()) raise ValueError(f"Unrecognized keyword arguments for extract: {kwargsStr}") - # must use list because we might be adding/deleting elements - for name, schemaItem in list(d.items()): - key = schemaItem.key - if split and key.HAS_NAMED_SUBFIELDS: - for subname, subkey in zip(key.subfields, key.subkeys): - d[f"{name}.{subname}"] = self.get(subkey) - del d[name] - else: - d[name] = self.get(schemaItem.key) - return d + return {name: self.get(schemaItem.key) for name, schemaItem in d.items()} def __repr__(self): return f"{type(self)}\n{self}" diff --git a/python/lsst/afw/table/_schema.cc b/python/lsst/afw/table/_schema.cc index bf971a9c31..86c13c7bdc 100644 --- a/python/lsst/afw/table/_schema.cc +++ b/python/lsst/afw/table/_schema.cc @@ -314,7 +314,6 @@ void declareSchemaType(WrapperCollection &wrappers) { // KeyBase wrappers.wrapType(PyKeyBase(wrappers.module, ("KeyBase" + suffix).c_str()), [](auto &mod, auto &cls) { - cls.def_readonly_static("HAS_NAMED_SUBFIELDS", &KeyBase::HAS_NAMED_SUBFIELDS); declareKeyBaseSpecializations(cls); }); diff --git a/src/table/KeyBase.cc b/src/table/KeyBase.cc index e317138ce7..a0c69e428a 100644 --- a/src/table/KeyBase.cc +++ b/src/table/KeyBase.cc @@ -16,14 +16,6 @@ Key::Element> KeyBase::getStorage() const { return detail::Access::extractElement(*this, 0); } -bool const KeyBase::HAS_NAMED_SUBFIELDS; - -template -bool const KeyBase::HAS_NAMED_SUBFIELDS; - -template -bool const KeyBase>::HAS_NAMED_SUBFIELDS; - template std::vector KeyBase>::extractVector(BaseRecord const &record) const { Key> const *self = static_cast> const *>(this); diff --git a/src/table/Schema.cc b/src/table/Schema.cc index 6718758cc9..32eff51af9 100644 --- a/src/table/Schema.cc +++ b/src/table/Schema.cc @@ -3,10 +3,6 @@ #include #include -#include "boost/mpl/and.hpp" -#include "boost/mpl/bool.hpp" -#include "boost/iterator/transform_iterator.hpp" -#include "boost/iterator/filter_iterator.hpp" #include "boost/preprocessor/seq/for_each.hpp" #include "boost/preprocessor/tuple/to_seq.hpp" @@ -106,203 +102,32 @@ class ItemFunctors { namespace detail { -//----- Finding a SchemaItem by field name ------------------------------------------------------------------ - -// This is easier to understand if you start reading from the bottom of this section, with -// SchemaImpl::find(std::string const &), then work your way up. - -namespace { - -// Given a SchemaItem for a regular field, look for a subfield with the given name. -// Return the index of the subfield (>= 0) on success, -1 on failure. -template -inline int findNamedSubfield( - SchemaItem const &item, std::string const &name, char delimiter, - boost::mpl::true_ * // whether a match is possible based on the type of T; computed by caller -) { - if (name.size() <= item.field.getName().size()) return -1; - - if ( // compare invocation is equivalent to "name.startswith(item.field.getName())" in Python - name.compare(0, item.field.getName().size(), item.field.getName()) == 0 && - name[item.field.getName().size()] == delimiter) { - int const position = item.field.getName().size() + 1; - int const size = name.size() - position; - int const nElements = item.field.getElementCount(); - for (int i = 0; i < nElements; ++i) { - if (name.compare(position, size, Key::subfields[i]) == 0) { - return i; - } - } - } - return -1; -} - -// This is an overload of findNamedSubfield that always fails; it's called when we -// know from the type of the field and subfield that they're incompatible. -template -inline int findNamedSubfield( - SchemaItem const &item, std::string const &name, char delimiter, - boost::mpl::false_ * // whether a match is possible based on the type of T; computed by caller -) { - return -1; -} - -// Given a SchemaItem and a subfield index, make a new SchemaItem that corresponds to that -// subfield and put it in the result smart pointer. -template -inline void makeSubfieldItem( - SchemaItem const &item, int index, char delimiter, std::unique_ptr > &result, - boost::mpl::true_ * // whether a match is possible based on the types of T and U; computed by caller -) { - result.reset(new SchemaItem(detail::Access::extractElement(item.key, index), - Field(join(item.field.getName(), Key::subfields[index], delimiter), - item.field.getDoc(), item.field.getUnits()))); -} - -// An overload of makeSubfieldItem that always fails because we know T and U aren't compatible. -template -inline void makeSubfieldItem( - SchemaItem const &item, int index, char delimiter, std::unique_ptr > &result, - boost::mpl::false_ * // whether a match is possible based on the types of T and U; computed by caller -) {} - -// This is a Variant visitation functor used to extract subfield items by name. -// For example, if we have a Point field "a", if we search the Schema for "a.x", -// we want to return a SchemaItem that makes it look like "a.x" is a full-fledged -// field in its own right. -template -struct ExtractItemByName : public boost::static_visitor<> { - explicit ExtractItemByName(std::string const &name_, char delimiter_) - : delimiter(delimiter_), name(name_) {} - - template - void operator()(SchemaItem const &item) const { - // We want to find out if 'item' has a subfield whose fully-qualified name matches our - // name data member. But we also know that the subfield needs to have type U, and that - // the field needs to have named subfields. - // This typedef is boost::mpl::true_ if all the above is true, and boost::mpl::false_ otherwise. - typedef typename boost::mpl::and_::Element>, - boost::mpl::bool_::HAS_NAMED_SUBFIELDS> >::type - IsMatchPossible; - // We use that type to dispatch one of the two overloads of findNamedSubfield. - int n = findNamedSubfield(item, name, delimiter, (IsMatchPossible *)0); - // If we have a match, we call another overloaded template to make the subfield. - if (n >= 0) makeSubfieldItem(item, n, delimiter, result, (IsMatchPossible *)0); - } - - char delimiter; - std::string name; // name we're looking for - mutable std::unique_ptr > result; // where we put the result to signal that we're done -}; - -} // namespace - -// Here's the driver for the find-by-name algorithm. template SchemaItem SchemaImpl::find(std::string const &name) const { NameMap::const_iterator i = _names.lower_bound(name); - if (i != _names.end()) { - if (i->first == name) { - // got an exact match; we're done if it has the right type, and dead if it doesn't. - try { - return boost::get const>(_items[i->second]); - } catch (boost::bad_get &err) { - throw LSST_EXCEPT(lsst::pex::exceptions::TypeError, - (boost::format("Field '%s' does not have the given type.") % name).str()); - } + if (i != _names.end() && i->first == name) { + // got an exact match; we're done if it has the right type, and dead if it doesn't. + try { + return boost::get>(_items[i->second]); + } catch (boost::bad_get &err) { + throw LSST_EXCEPT(lsst::pex::exceptions::TypeError, + (boost::format("Field '%s' does not have the given type.") % name).str()); } } - // We didn't get an exact match, but we might be searching for "a.x/a_x" and "a" might be a point field. - // Because the names are sorted, we know we overshot it, so we work backwards. - ExtractItemByName extractor(name, getDelimiter()); - while (i != _names.begin()) { - --i; - boost::apply_visitor(extractor, _items[i->second]); // see if the current item is a match - if (extractor.result) return *extractor.result; - } throw LSST_EXCEPT(lsst::pex::exceptions::NotFoundError, - (boost::format("Field or subfield with name '%s' not found with type '%s'.") % name % + (boost::format("Field with name '%s' not found with type '%s'.") % name % Field::getTypeString()) .str()); } -//----- Finding a SchemaItem by key ------------------------------------------------------------------------- - -// This is easier to understand if you start reading from the bottom of this section, with -// SchemaImpl::find(Key const &), then work your way up. - -namespace { - -// Given a SchemaItem for a regular field, look for a subfield with the given Key -// Return the index of the subfield (>= 0) on success, -1 on failure. -template -inline int findKeySubfield( - SchemaItem const &item, Key const &key, - boost::mpl::true_ * // whether a match is possible based on the types of T and U; computed by caller -) { - int n = (key.getOffset() - item.key.getOffset()) / sizeof(U); - if (n >= 0 && n < item.key.getElementCount()) { - return n; - } - return -1; -} - -// This is an overload of findKeySubfield that always fails; it's called when we -// know from the type of the field and subfield that they're incompatible. -template -inline int findKeySubfield( - SchemaItem const &item, Key const &key, - boost::mpl::false_ * // whether a match is possible based on the types of T and U; computed by caller -) { - return -1; -} - -// This is a Variant visitation functor used to extract subfield items by key. -template -struct ExtractItemByKey : public boost::static_visitor<> { - explicit ExtractItemByKey(Key const &key_, char delimiter_) : delimiter(delimiter_), key(key_) {} - - template - void operator()(SchemaItem const &item) const { - // We want to find out if 'item' has a subfield whose matches our key data member. - // But we also know that the subfield needs to have type U. - // This typedef is boost::mpl::true_ if the above is true, and boost::mpl::false_ otherwise. - typedef typename boost::mpl::and_::Element>, - boost::mpl::bool_::HAS_NAMED_SUBFIELDS> >::type - IsMatchPossible; - // We use that type to dispatch one of the two overloads of findKeySubfield. - int n = findKeySubfield(item, key, (IsMatchPossible *)0); - // If we have a match, we call another overloaded template to make the subfield. - // (this is the same makeSubfieldItem used in ExtractItemByName, so it's defined up there) - if (n >= 0) makeSubfieldItem(item, n, delimiter, result, (IsMatchPossible *)0); - } - - char delimiter; - Key key; - mutable std::unique_ptr > result; -}; - -} // namespace - -// Here's the driver for the find-by-key algorithm. It's pretty similar to the find-by-name algorithm. template SchemaItem SchemaImpl::find(Key const &key) const { OffsetMap::const_iterator i = _offsets.lower_bound(key.getOffset()); - if (i != _offsets.end()) { - if (i->first == key.getOffset()) { - try { - return boost::get const>(_items[i->second]); - } catch (boost::bad_get &err) { - // just swallow the exception; this might be a subfield key that points to the beginning. - } - } - // We didn't get an exact match, but we might be searching for a subfield. - // Because the offsets are sorted, we know we overshot it, so we work backwards. - ExtractItemByKey extractor(key, getDelimiter()); - while (i != _offsets.begin()) { - --i; - boost::apply_visitor(extractor, _items[i->second]); - if (extractor.result) return *extractor.result; + if (i != _offsets.end() && i->first == key.getOffset()) { + try { + return boost::get>(_items[i->second]); + } catch (boost::bad_get &err) { + // just swallow the exception; this might be a subfield key that points to the beginning. } } throw LSST_EXCEPT(lsst::pex::exceptions::NotFoundError,