Hello. We're big fans of Eigen and use it extensively. The comma initialization feature is very nice, but I'm wondering why implicit conversion from CommaInitializer to XprType is not used to avoid the need for .finished() when using comma initialization as a temporary object?

To test this, I wrote a "special" CommaInitializer class. This seems to work fine, and enables syntax like:

Code: Select all

typedef Eigen::VectorXd Vector;
Vector vector = (Vector(3) << 1, 2, 3);    // Initialization from a temporary
functionCall(a, b, (Vector() << 1, 2, 3)); // Passing into a function


Here is the code for SpecialCommaInitializer:

Code: Select all

template<typename XprType>
struct SpecialCommaInitializer : public Eigen::CommaInitializer<XprType>
{
  typedef Eigen::CommaInitializer<XprType> Base;

  // Forward to base class
  inline SpecialCommaInitializer(XprType& xpr, const typename XprType::Scalar& s) :
      Base(xpr, s) {}

  // Forward to base class
  template<typename OtherDerived>
  inline SpecialCommaInitializer(XprType& xpr, const Eigen::DenseBase<OtherDerived>& other) :
      Base(xpr, other) {}

  /// Implicit conversion to expression type, e.g. Vector or Matrix
  inline operator XprType () { return this->finished(); }

  /// Override base class comma operators to return this class instead of the base class.
  SpecialCommaInitializer& operator,(const typename XprType::Scalar& s) {
    (void) Base::operator,(s);
    return *this;
  }

  /// Override base class comma operators to return this class instead of the base class.
  template<typename OtherDerived>
  SpecialCommaInitializer& operator,(const Eigen::DenseBase<OtherDerived>& other) {
    (void) Base::operator,(other);
    return *this;
  }
};


To test out the above code, the DenseBase<Derived>::operator<< implementations in Core/CommaInitializer.h should me modified to return SpecialCommaInitializer instead of CommaInitializer.

I wondered if not allowing the syntax without .finished() was a design decision?

P.S. I'd be glad to supply a patch for CommaInitializer that enables this implicit conversion if it makes sense to include this feature.

Thanks and best regards,
Richard Roberts

This seems to be a nice addition. However, the .finished() is still needed to be able to call members or operators. A proper patch with a unit test and updated documentation is very welcome!

hm, actually this trick is quite limited because it also won't work with templated functions. One solution would be to make CommaInitializer inherits MatrixBase. The only drawback is that I don't see how to check the matrix has been completely filled.

That's a good point - the only reason the trick works for us is that we rarely use templated functions that accept DenseBase, we normally just use VectorXd and MatrixXd. If CommaInitializer was inherited from DenseBase, and we need to know if the matrix is fully initialized, I think it work to check the m_row and m_col members against the size of m_xpr.

Can you think of an easy way to derive CommaInitializer from DenseBase, such that it's not necessary to implement the whole DenseBase interface, and just have some kind of implicit conversion? I tried adding conversion operators to DenseBase<XprType> but this doesn't work.

A good example to see how to make CommaInitializer a true expression is the Flagged class (src/Core/Flagged.h). Basically you only have to specialize internal::traits for CommaInitializer and make it inherits traits<XprType>, and in CommaInitializer a few members: rows/cols/coeff*/*packet* that are simply forwarded to the nested matrix object.

Great, this works for templated functions now. I tested with functions of the form fcn(const DenseBase<Derived>&) and fcn(const MatrixBase<Derived>&). Below is the new code for SpecialCommaInitializer that inherits from MatrixBase. Please let me know if you notice any mistakes.

We will use this in-house here for a little while to try to work out any problems and then I will submit a patch for the real CommaInitializer when I'm relatively certain it doesn't cause any problems.

Code: Select all

template<typename XprType>
class SpecialCommaInitializer :
    public Eigen::CommaInitializer<XprType>,
    public Eigen::MatrixBase<SpecialCommaInitializer<XprType> >
{
public:
  typedef Eigen::MatrixBase<SpecialCommaInitializer<XprType> > Base;
  typedef Eigen::CommaInitializer<XprType> CommaBase;

  EIGEN_DENSE_PUBLIC_INTERFACE(SpecialCommaInitializer)
  typedef typename Eigen::internal::conditional<Eigen::internal::must_nest_by_value<XprType>::ret,
      XprType, const XprType&>::type ExpressionTypeNested;
  typedef typename XprType::InnerIterator InnerIterator;

  // Forward to base class
  inline SpecialCommaInitializer(XprType& xpr, const typename XprType::Scalar& s) :
      CommaBase(xpr, s) {}

  // Forward to base class
  template<typename OtherDerived>
  inline SpecialCommaInitializer(XprType& xpr, const Eigen::DenseBase<OtherDerived>& other) :
      CommaBase(xpr, other) {}

  inline Index rows() const { return CommaBase::m_xpr.rows(); }
  inline Index cols() const { return CommaBase::m_xpr.cols(); }
  inline Index outerStride() const { return CommaBase::m_xpr.outerStride(); }
  inline Index innerStride() const { return CommaBase::m_xpr.innerStride(); }

  inline CoeffReturnType coeff(Index row, Index col) const
  {
    return CommaBase::m_xpr.coeff(row, col);
  }

  inline CoeffReturnType coeff(Index index) const
  {
    return CommaBase::m_xpr.coeff(index);
  }

  inline const Scalar& coeffRef(Index row, Index col) const
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(row, col);
  }

  inline const Scalar& coeffRef(Index index) const
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(index);
  }

  inline Scalar& coeffRef(Index row, Index col)
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(row, col);
  }

  inline Scalar& coeffRef(Index index)
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(index);
  }

  template<int LoadMode>
  inline const PacketScalar packet(Index row, Index col) const
  {
    return CommaBase::m_xpr.template packet<LoadMode>(row, col);
  }

  template<int LoadMode>
  inline void writePacket(Index row, Index col, const PacketScalar& x)
  {
    CommaBase::m_xpr.const_cast_derived().template writePacket<LoadMode>(row, col, x);
  }

  template<int LoadMode>
  inline const PacketScalar packet(Index index) const
  {
    return CommaBase::m_xpr.template packet<LoadMode>(index);
  }

  template<int LoadMode>
  inline void writePacket(Index index, const PacketScalar& x)
  {
    CommaBase::m_xpr.const_cast_derived().template writePacket<LoadMode>(index, x);
  }

  const XprType& _expression() const { return CommaBase::m_xpr; }

  /// Override base class comma operators to return this class instead of the base class.
  SpecialCommaInitializer& operator,(const typename XprType::Scalar& s)
  {
    (void) CommaBase::operator,(s);
    return *this;
  }

  /// Override base class comma operators to return this class instead of the base class.
  template<typename OtherDerived>
  SpecialCommaInitializer& operator,(const Eigen::DenseBase<OtherDerived>& other)
  {
    (void) CommaBase::operator,(other);
    return *this;
  }
};

namespace internal {
template<typename ExpressionType>
struct traits<SpecialCommaInitializer<ExpressionType> > : traits<ExpressionType>
{
};
}


oh, maybe Flagged was not perfect example since it is for MatrixBase only. To make it work with both Array* and Matrix*, you need and additional trick to decide whether the base class is MatrixBase or ArrayBase. I also added the data() member: (not tested so there might be typos)

Code: Select all

template<typename XprType>
class SpecialCommaInitializer :
    public Eigen::CommaInitializer<XprType>,
    public internal::dense_xpr_base<XprType>::type
{
public:
  typedef typename internal::TransposeImpl_base<XprType>::type Base;
  typedef Eigen::CommaInitializer<XprType> CommaBase;

  EIGEN_DENSE_PUBLIC_INTERFACE(SpecialCommaInitializer)
  typedef typename XprType::InnerIterator InnerIterator;

  // Forward to base class
  inline SpecialCommaInitializer(XprType& xpr, const typename XprType::Scalar& s) :
      CommaBase(xpr, s) {}

  // Forward to base class
  template<typename OtherDerived>
  inline SpecialCommaInitializer(XprType& xpr, const Eigen::DenseBase<OtherDerived>& other) :
      CommaBase(xpr, other) {}

  inline Index rows() const { return CommaBase::m_xpr.rows(); }
  inline Index cols() const { return CommaBase::m_xpr.cols(); }

  inline       Scalar* data()       { return CommaBase::m_xpr.data(); }
  inline const Scalar* data() const { return CommaBase::m_xpr.data(); }
  inline Index outerStride() const  { return CommaBase::m_xpr.outerStride(); }
  inline Index innerStride() const  { return CommaBase::m_xpr.innerStride(); }

  inline CoeffReturnType coeff(Index row, Index col) const
  {
    return CommaBase::m_xpr.coeff(row, col);
  }

  inline CoeffReturnType coeff(Index index) const
  {
    return CommaBase::m_xpr.coeff(index);
  }

  inline const Scalar& coeffRef(Index row, Index col) const
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(row, col);
  }

  inline const Scalar& coeffRef(Index index) const
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(index);
  }

  inline Scalar& coeffRef(Index row, Index col)
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(row, col);
  }

  inline Scalar& coeffRef(Index index)
  {
    return CommaBase::m_xpr.const_cast_derived().coeffRef(index);
  }

  template<int LoadMode>
  inline const PacketScalar packet(Index row, Index col) const
  {
    return CommaBase::m_xpr.template packet<LoadMode>(row, col);
  }

  template<int LoadMode>
  inline void writePacket(Index row, Index col, const PacketScalar& x)
  {
    CommaBase::m_xpr.const_cast_derived().template writePacket<LoadMode>(row, col, x);
  }

  template<int LoadMode>
  inline const PacketScalar packet(Index index) const
  {
    return CommaBase::m_xpr.template packet<LoadMode>(index);
  }

  template<int LoadMode>
  inline void writePacket(Index index, const PacketScalar& x)
  {
    CommaBase::m_xpr.const_cast_derived().template writePacket<LoadMode>(index, x);
  }

  const XprType& _expression() const { return CommaBase::m_xpr; }

  /// Override base class comma operators to return this class instead of the base class.
  SpecialCommaInitializer& operator,(const typename XprType::Scalar& s)
  {
    (void) CommaBase::operator,(s);
    return *this;
  }

  /// Override base class comma operators to return this class instead of the base class.
  template<typename OtherDerived>
  SpecialCommaInitializer& operator,(const Eigen::DenseBase<OtherDerived>& other)
  {
    (void) CommaBase::operator,(other);
    return *this;
  }
};

namespace internal {
template<typename ExpressionType>
struct traits<SpecialCommaInitializer<ExpressionType> > : traits<ExpressionType>
{
};
}


Cool! Is it a typo that the base class is 'internal::dense_xpr_base<XprType>::type' but the Base typedef is 'typename internal::TransposeImpl_base<XprType>::type' ?

oops, copy paste mistake! It must be:

Code: Select all

template<typename XprType>
class SpecialCommaInitializer :
    public Eigen::CommaInitializer<XprType>,
    public internal::dense_xpr_base<SpecialCommaInitializer<XprType> >::type
{
public:
typedef internal::dense_xpr_base<SpecialCommaInitializer<XprType> >::type Base;
...