Hi,

I previously found a note on this forum asking for additional binary ops capability in the Array class. I can't seem to find it now, but the reply from one of the core developers was "patch welcome". I've done a little cut and paste trial implementation and would like some guidance on how to flesh it out to make it acceptable.

My trial operator is ">>".

I've created an include "ArrayBaseAddons.h" with:

Code: Select all

inline const CwiseUnaryOp<internal::scalar_shr_op<Scalar>, const Derived>
operator>>(const Scalar& scalar) const
{
  return CwiseUnaryOp<internal::scalar_shr_op<Scalar>, const Derived>(derived(), internal::scalar_shr_op<Scalar>(scalar));
}

friend inline const CwiseUnaryOp<internal::scalar_shr_op<Scalar>, const Derived>
operator>>(const Scalar& scalar,const EIGEN_CURRENT_STORAGE_BASE_CLASS<Derived>& other)
{
  return other >> scalar;
}



I don't understand how to add something to "namespace internal" from another file so I've hacked on the Eigen/src/Core/Functors.h adding:

Code: Select all

/** \internal
  * \brief Template functor to shr a scalar to a fixed other one
  * \sa class CwiseUnaryOp, Array::operator>>
  */
/* If you wonder why doing the pset1() in packetOp() is an optimization check scalar_multiple_op */
template<typename Scalar>
struct scalar_shr_op {
  typedef typename packet_traits<Scalar>::type Packet;
  // FIXME default copy constructors seems bugged with std::complex<>
  inline scalar_shr_op(const scalar_shr_op& other) : m_other(other.m_other) { }
  inline scalar_shr_op(const Scalar& other) : m_other(other) { }
  inline Scalar operator() (const Scalar& a) const { return a >> m_other; }
  inline const Packet packetOp(const Packet& a) const
  { return internal::padd(a, pset1<Packet>(m_other)); }
  const Scalar m_other;
};
template<typename Scalar>
struct functor_traits<scalar_shr_op<Scalar> >
{ enum { Cost = NumTraits<Scalar>::AddCost, PacketAccess = packet_traits<Scalar>::HasAdd }; };



A very quick test shows that this at least compiles and does the right thing.

It seems that I'll have to change the padd above and add something to the generic packet code also, but thats an optimization.

I'd be very happy to add the rest of the integer operators with test cases and proper style given a little guidance.

Is this the correct direction for these additions?
What other files should I be looking at?
How is testing done in Eigen (I'll look at the online docs more later today)?

Why is "CwiseUnaryOp" used here to add binary ops?

Thanks,

John

Some quick comments ...

I assume you define EIGEN_ARRAYBASE_PLUGIN to include ArrayBaseAddons.h. If you want to produce a patch then you should put it with the other operators in ArrayCwiseUnaryOps.h (which is in the Eigen/src/plugins directory and easy to miss). That way we keep EIGEN_ARRAYBASE_PLUGIN available for users.

The friend operator>> function you define in ArrayBaseAddons.h is called on expressions like "scalar >> array". Your implementation assumes that scalar >> array and array >> scalar are the same; this is true for + but not for >> .

If you can't implement scalar_shr_op::packetOp() correctly then set PacketAccess in the functor_traits to zero; packetOp() is only called if PacketAccess is true.

For testing in Eigen, see http://eigen.tuxfamily.org/index.php?title=Tests .
You'll also need to write documentation and examples; there are some tips in http://eigen.tuxfamily.org/index.php?ti ... ation_tips .

In Eigen, an operation is called unary if it acts on one matrix or array argument and binary if it acts on two matrix or array arguments. So, the multiplication in "scalar * array" is a unary operation, while it is a binary operation in "array * array".

Thanks for the feedback. I'll post a patch, not a plugin, on my next iteration. I've got hg loaded and will read about CMake.

I'll set PacketAccess to 0 for now and look at it when the structure of the patch is more settled.

I've read the section on testing, thanks.

Thanks for the explanation of Unary, everything is named fairly clearly and that seemed strange.

John