ipa: libipa: vector: Add matrix-vector multiplication
diff mbox series

Message ID 20240607082045.2718555-1-paul.elder@ideasonboard.com
State New
Headers show
Series
  • ipa: libipa: vector: Add matrix-vector multiplication
Related show

Commit Message

Paul Elder June 7, 2024, 8:20 a.m. UTC
Add an operation for multiplying a matrix with a vector.

Signed-off-by: Paul Elder <paul.elder@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(+)

Comments

Kieran Bingham June 10, 2024, 11:53 a.m. UTC | #1
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@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)
> +{
> +       Vector<T, R1> result;
> +

Does this need to ASSERT(C1 == R2); or such?

Other than that it looks ok to me, but I haven't seen the usage yet (or
tests :D)


Reviewed-by: Kieran Bingham <kieran.bingham@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>
> -- 
> 2.39.2
>
Paul Elder June 10, 2024, 2:25 p.m. UTC | #2
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@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)
> > +{
> > +       Vector<T, R1> result;
> > +
> 
> Does this need to ASSERT(C1 == R2); or such?

It's in SFINAE... is that not sufficient?

> 
> Other than that it looks ok to me, but I haven't seen the usage yet (or
> tests :D)
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Thanks,

Paul

> 
> 
> > +       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>
> > -- 
> > 2.39.2
> >
Kieran Bingham June 10, 2024, 2:52 p.m. UTC | #3
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@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)
> > > +{
> > > +       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.

--
Kieran


> 
> > 
> > Other than that it looks ok to me, but I haven't seen the usage yet (or
> > tests :D)
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> Thanks,
> 
> Paul
> 
> > 
> > 
> > > +       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>
> > > -- 
> > > 2.39.2
> > >
Laurent Pinchart June 11, 2024, 10:55 p.m. UTC | #4
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@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@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>

Patch
diff mbox series

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)
+{
+	Vector<T, R1> result;
+
+	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>