[PATCH] ipa: libipa: vector: Add matrix-vector multiplication

[Laurent Pinchart]

On Mon, Jun 10, 2024 at 03:52:28PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-06-10 15:25:01)
> > On Mon, Jun 10, 2024 at 12:53:22PM +0100, Kieran Bingham wrote:
> > > Quoting Paul Elder (2024-06-07 09:20:45)
> > > > Add an operation for multiplying a matrix with a vector.
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > 
> > > > ---
> > > > Depends on v6 of "ipa: libipa: Add Matrix class" and v5 of "ipa: libipa:
> > > > Add Vector class"
> > > > ---
> > > >  src/ipa/libipa/vector.cpp | 12 ++++++++++++
> > > >  src/ipa/libipa/vector.h   | 22 ++++++++++++++++++++++
> > > >  2 files changed, 34 insertions(+)
> > > > 
> > > > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > > > index bdc3c447c..e18426fe5 100644
> > > > --- a/src/ipa/libipa/vector.cpp
> > > > +++ b/src/ipa/libipa/vector.cpp
> > > > @@ -145,6 +145,18 @@ namespace ipa {
> > > >   * \return The length of the vector
> > > >   */
> > > >  
> > > > +/**
> > > > + * \fn Vector<T, R1> operator*(const Matrix<T, R1, C1> &m, const Vector<T, R2> &v)
> > > > + * \brief Multiply a matrix by a vector
> > > > + * \tparam T Numerical type of the contents of the matrix and vector
> > > > + * \tparam R1 The number of rows in the matrix
> > > > + * \tparam C1 The number of colums in the matrix
> > > > + * \tparam R2 The number of elements in the vector
> > > > + * \param m The matrix
> > > > + * \param v The vector
> > > > + * \return Product of matrix \a m and vector \a v
> > > > + */
> > > > +
> > > >  /**
> > > >   * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > > >   * \brief Compare vectors for equality
> > > > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > > > index 87aad28b5..934b44494 100644
> > > > --- a/src/ipa/libipa/vector.h
> > > > +++ b/src/ipa/libipa/vector.h
> > > > @@ -17,6 +17,8 @@
> > > >  
> > > >  #include "libcamera/internal/yaml_parser.h"
> > > >  
> > > > +#include "matrix.h"
> > > > +
> > > >  namespace libcamera {
> > > >  
> > > >  LOG_DECLARE_CATEGORY(Vector)
> > > > @@ -166,6 +168,26 @@ private:
> > > >         std::array<T, R> data_;
> > > >  };
> > > >  
> > > > +#ifndef __DOXYGEN__
> > > > +template<typename T,
> > > > +        unsigned int R1, unsigned int C1,
> > > > +        unsigned int R2,
> > > > +        std::enable_if_t<std::is_arithmetic_v<T> && C1 == R2> * = nullptr>
> > > > +#endif /* __DOXYGEN__ */
> > > > +Vector<T, R1> operator*(const Matrix<T, R1, C1> &m, const Vector<T, R2> &v)

I think you can simplify all this as

template<typename T, unsigned int Rows, unsigned int Cols>
Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v)

> > > > +{
> > > > +       Vector<T, R1> result;
> > > > +
> > > 
> > > Does this need to ASSERT(C1 == R2); or such?
> > 
> > It's in SFINAE... is that not sufficient?
> 
> Ah, you mean this line above:
> 
>        std::enable_if_t<std::is_arithmetic_v<T> && C1 == R2> * = nullptr>
> 
> I missed that. If it provides the compile time guarantees, then sure -
> even better.
> 
> > > Other than that it looks ok to me, but I haven't seen the usage yet (or
> > > tests :D)

Is it used in any series already posted that I have missed, or should I
just wait ?

I think we need unit tests for this (as well as the Vector and Matrix
classes). We don't have to go overboard and test every trivial functions
in the initial implementation. Not a big blocker, but it should be
recorded somewhere and added.

> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > +       for (unsigned int i = 0; i < R1; i++) {
> > > > +               T sum = 0;
> > > > +               for (unsigned int j = 0; j < C1; j++)
> > > > +                       sum += m[i][j] * v[j];
> > > > +               result[i] = sum;
> > > > +       }
> > > > +
> > > > +       return result;
> > > > +}
> > > > +
> > > >  #ifndef __DOXYGEN__
> > > >  template<typename T, unsigned int R,
> > > >          std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>

-- 
Regards,

Laurent Pinchart