Re: [eigen] [PATCH] adding Reverse expression

On Tue, Feb 3, 2009 at 12:50 AM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:

Thanks for your patch; comments below:

2009/2/3 Ricard Marxer Piñón <email@xxxxxxxxxxxxxxxx>:

> Here is a patch to add the Reverse _expression_ this is useful for flipping
> matrices, rows or columns.

I can see that you declare a class Reverse, but don't actually define
it. You probably forgot to "svn add Reverse.h" ?

ups yep. I had some trouble because I still had all the Toeplitz stuff and was trying to isolate the reverse stuff (aaah I wish I could get git svn to work)
Anyway here is the full diff.

Your PartialRedux::reverse() method returns a ExpressionType which has
2 drawbacks:
1) it requires ExpressionType to be writable, e.g. one can't use your
reverse() like this,
m3 = (m1+m2).rowwise().reverse();
2) more importantly, your PartialRedux::reverse() is not lazy, so when one does
v2 = v1.reverse();
the arrays are traversed twice: first a temporary vector is created by
reverse(), then this temporary vector is copied into v2.
This is of course inefficient. It is exactly in order to overcome
that, that we introduct _expression_ templates ;)

Yes the PartialRedux reverse is very drafty, I just wanted the API to be as I wanted it to be. I will try to get it right with a bit more work.

Cheers,
Benoit

Index: doc/snippets/PartialRedux_reverse.cpp =================================================================== --- doc/snippets/PartialRedux_reverse.cpp (revision 0) +++ doc/snippets/PartialRedux_reverse.cpp (revision 0) @@ -0,0 +1,4 @@ +MatrixXi m = MatrixXi::Random(3,4); +cout << "Here is the matrix m:" << endl << m << endl; +cout << "Here is the rowwise reverse of m:" << endl << m.rowwise().reverse() << endl; +cout << "Here is the colwise reverse of m:" << endl << m.colwise().reverse() << endl; Index: doc/snippets/MatrixBase_reverse.cpp =================================================================== --- doc/snippets/MatrixBase_reverse.cpp (revision 0) +++ doc/snippets/MatrixBase_reverse.cpp (revision 0) @@ -0,0 +1,8 @@ +MatrixXi m = MatrixXi::Random(3,4); +cout << "Here is the matrix m:" << endl << m << endl; +cout << "Here is the reverse of m:" << endl << m.reverse() << endl; +cout << "Here is the coefficient (1,0) in the reverse of m:" << endl + << m.reverse()(1,0) << endl; +cout << "Let us overwrite this coefficient with the value 4." << endl; +m.reverse()(1,0) = 4; +cout << "Now the matrix m is:" << endl << m << endl; Index: Eigen/src/Core/Reverse.h =================================================================== --- Eigen/src/Core/Reverse.h (revision 0) +++ Eigen/src/Core/Reverse.h (revision 0) @@ -0,0 +1,169 @@ +// This file is part of Eigen, a lightweight C++ template library +// for linear algebra. Eigen itself is part of the KDE project. +// +// Copyright (C) 2006-2008 Benoit Jacob <jacob.benoit.1@xxxxxxxxx> +// Copyright (C) 2006-2008 Ricard Marxer <email@xxxxxxxxxxxxxxxx> +// +// Eigen is free software; you can redistribute it and/or +// modify it under the terms of the GNU Lesser General Public +// License as published by the Free Software Foundation; either +// version 3 of the License, or (at your option) any later version. +// +// Alternatively, you can redistribute it and/or +// modify it under the terms of the GNU General Public License as +// published by the Free Software Foundation; either version 2 of +// the License, or (at your option) any later version. +// +// Eigen is distributed in the hope that it will be useful, but WITHOUT ANY +// WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +// FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License or the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public +// License and a copy of the GNU General Public License along with +// Eigen. If not, see <http://www.gnu.org/licenses/>. + +#ifndef EIGEN_REVERSE_H +#define EIGEN_REVERSE_H + +/** \class Reverse + * + * \brief Expression of the reverse of a vector + * + * \param MatrixType the type of the object of which we are taking the reverse + * + * This class represents an expression of the reverse of a vector. + * It is the return type of MatrixBase::reverse() + * and most of the time this is the only way it is used. + * + * \sa MatrixBase::reverse() + */ +template<typename MatrixType> +struct ei_traits<Reverse<MatrixType> > +{ + typedef typename MatrixType::Scalar Scalar; + typedef typename ei_nested<MatrixType>::type MatrixTypeNested; + typedef typename ei_unref<MatrixTypeNested>::type _MatrixTypeNested; + enum { + RowsAtCompileTime = MatrixType::RowsAtCompileTime, + ColsAtCompileTime = MatrixType::ColsAtCompileTime, + MaxRowsAtCompileTime = MatrixType::MaxRowsAtCompileTime, + MaxColsAtCompileTime = MatrixType::MaxColsAtCompileTime, + + // TODO: check how to correctly set the new flags + Flags = ((int(_MatrixTypeNested::Flags) ^ RowMajorBit) + & ~(LowerTriangularBit | UpperTriangularBit)) + | (int(_MatrixTypeNested::Flags)&UpperTriangularBit ? LowerTriangularBit : 0) + | (int(_MatrixTypeNested::Flags)&LowerTriangularBit ? UpperTriangularBit : 0), + + // TODO: should add two index type AddCosts (due to the rows - i - 1) or only one, and add the cost of calling .rows() and .cols() + CoeffReadCost = _MatrixTypeNested::CoeffReadCost + }; +}; + +template<typename MatrixType> class Reverse + : public MatrixBase<Reverse<MatrixType> > +{ + public: + + EIGEN_GENERIC_PUBLIC_INTERFACE(Reverse) + + inline Reverse(const MatrixType& matrix) : m_matrix(matrix) {} + + EIGEN_INHERIT_ASSIGNMENT_OPERATORS(Reverse) + + inline int rows() const { return m_matrix.rows(); } + inline int cols() const { return m_matrix.cols(); } + inline int nonZeros() const { return m_matrix.nonZeros(); } + inline int stride(void) const { return m_matrix.stride(); } + + inline Scalar& coeffRef(int row, int col) + { + return m_matrix.const_cast_derived().coeffRef(m_matrix.rows() - row - 1, m_matrix.cols() - col - 1); + } + + inline const Scalar coeff(int row, int col) const + { + return m_matrix.coeff(m_matrix.rows() - row - 1, m_matrix.cols() - col - 1); + } + + inline const Scalar coeff(int index) const + { + return m_matrix.coeff(m_matrix.rows() * m_matrix.cols() - index - 1); + } + + inline Scalar& coeffRef(int index) + { + return m_matrix.const_cast_derived().coeffRef(m_matrix.rows() * m_matrix.cols() - index - 1); + } + + // TODO: We must reverse the packet reading and writing, which is currently not done here, I think + template<int LoadMode> + inline const PacketScalar packet(int row, int col) const + { + return m_matrix.template packet<LoadMode>(m_matrix.rows() - row - 1, m_matrix.cols() - col - 1); + } + + template<int LoadMode> + inline void writePacket(int row, int col, const PacketScalar& x) + { + m_matrix.const_cast_derived().template writePacket<LoadMode>(m_matrix.rows() - row - 1, m_matrix.cols() - col - 1, x); + } + + template<int LoadMode> + inline const PacketScalar packet(int index) const + { + return m_matrix.template packet<LoadMode>(m_matrix.rows() * m_matrix.cols() - index - 1); + } + + template<int LoadMode> + inline void writePacket(int index, const PacketScalar& x) + { + m_matrix.const_cast_derived().template writePacket<LoadMode>(m_matrix.rows() * m_matrix.cols() - index - 1, x); + } + + protected: + const typename MatrixType::Nested m_matrix; +}; + +/** \returns an expression of the reverse of *this. + * + * Example: \include MatrixBase_reverse.cpp + * Output: \verbinclude MatrixBase_reverse.out + * + */ +template<typename Derived> +inline Reverse<Derived> +MatrixBase<Derived>::reverse() +{ + return derived(); +} + +/** This is the const version of reverse(). */ +template<typename Derived> +inline const Reverse<Derived> +MatrixBase<Derived>::reverse() const +{ + return derived(); +} + +/** This is the "in place" version of reverse: it reverses \c *this. + * + * In most cases it is probably better to simply use the reversed expression + * of a matrix. However, when reversing the matrix data itself is really needed, + * then this "in-place" version is probably the right choice because it provides + * the following additional features: + * - less error prone: doing the same operation with .reverse() requires special care: + * \code m = m.reverse().eval(); \endcode + * - no temporary object is created (currently there is one created but could be avoided using swap) + * - it allows future optimizations (cache friendliness, etc.) + * + * \sa reverse() */ +template<typename Derived> +inline void MatrixBase<Derived>::reverseInPlace() +{ + this = this.reverse().eval(); +} + + +#endif // EIGEN_REVERSE_H Index: Eigen/src/Core/MatrixBase.h =================================================================== --- Eigen/src/Core/MatrixBase.h (revision 920471) +++ Eigen/src/Core/MatrixBase.h (working copy) @@ -359,8 +359,11 @@ const Eigen::Transpose<Derived> transpose() const; void transposeInPlace(); const AdjointReturnType adjoint() const; + + Eigen::Reverse<Derived> reverse(); + const Eigen::Reverse<Derived> reverse() const; + void reverseInPlace(); - RowXpr row(int i); const RowXpr row(int i) const; Index: Eigen/src/Core/util/ForwardDeclarations.h =================================================================== --- Eigen/src/Core/util/ForwardDeclarations.h (revision 920471) +++ Eigen/src/Core/util/ForwardDeclarations.h (working copy) @@ -40,6 +40,7 @@ int _DirectAccessStatus = ei_traits<MatrixType>::Flags&DirectAccessBit ? DirectAccessBit : ei_traits<MatrixType>::Flags&SparseBit> class Block; template<typename MatrixType> class Transpose; +template<typename MatrixType> class Reverse; template<typename MatrixType> class Conjugate; template<typename NullaryOp, typename MatrixType> class CwiseNullaryOp; template<typename UnaryOp, typename MatrixType> class CwiseUnaryOp; Index: Eigen/src/Array/PartialRedux.h =================================================================== --- Eigen/src/Array/PartialRedux.h (revision 920471) +++ Eigen/src/Array/PartialRedux.h (working copy) @@ -258,6 +258,34 @@ const PartialReduxExpr<ExpressionType, ei_member_count<int>, Direction> count() const { return _expression(); } + + /** \returns a matrix expression + * where each column (or row) are reversed. + * + * Example: \include PartialRedux_reverse.cpp + * Output: \verbinclude PartialRedux_reverse.out + * + * \sa MatrixBase::reverse() */ + const ExpressionType reverse() const + { + const int rows = _expression().rows(); + const int cols = _expression().cols(); + + ExpressionType mat(rows, cols); + + if(Direction==Vertical) { + for (int i = 0; i < cols; i++) { + mat.col(i) = _expression().col(i).reverse(); + } + } else { + for (int i = 0; i < rows; i++) { + mat.row(i) = _expression().row(i).reverse(); + } + } + + return mat; + } + /** \returns a 3x3 matrix expression of the cross product * of each column or row of the referenced expression with the \a other vector. * Index: Eigen/Core =================================================================== --- Eigen/Core (revision 920471) +++ Eigen/Core (working copy) @@ -136,6 +136,7 @@ #include "src/Core/Block.h" #include "src/Core/Minor.h" #include "src/Core/Transpose.h" +#include "src/Core/Reverse.h" #include "src/Core/DiagonalMatrix.h" #include "src/Core/DiagonalCoeffs.h" #include "src/Core/Sum.h"