Merge pull request #3327 from rcurtin/tmatrix-binding-fixes

Fix binding handling of transposed matrix parameters

HISTORY.md

@@ -1,7 +1,7 @@
 ### mlpack ?.?.?
 ###### ????-??-??
   * Fix mapping of categorical data for Julia bindings (#3305).
-  
+
   * Bugfix: catch all exceptions when running bindings from Julia, instead of
     crashing (#3304).
 
@@ -13,11 +13,14 @@
 
   * The `/std:c++17` and `/Zc:__cplusplus` options are now required when using
     Visual Studio (#3318).  Documentation and compile-time checks added.
-    
+
   * Set `BUILD_TESTS` to `OFF` by default.  If you want to build tests, like
     `mlpack_test`, manually set `BUILD_TESTS` to `ON` in your CMake
     configuration step (#3316).
 
+  * Fix handling of transposed matrix parameters in Python, Julia, R, and Go
+    bindings (#3327).
+
 ### mlpack 4.0.0
 ###### 2022-10-23
   * Bump C++ standard requirement to C++14 (#3233).

src/mlpack/bindings/R/mlpack/src/r_util.cpp

@@ -112,10 +112,11 @@ void SetParamVecInt(SEXP params,
 // [[Rcpp::export]]
 void SetParamMat(SEXP params,
                  const std::string& paramName,
-                 const arma::mat& paramValue)
+                 const arma::mat& paramValue,
+                 bool transpose)
 {
   util::Params& p = *Rcpp::as<Rcpp::XPtr<util::Params>>(params);
-  p.Get<arma::mat>(paramName) = paramValue.t();
+  p.Get<arma::mat>(paramName) = (transpose ? paramValue.t() : paramValue);
   p.SetPassed(paramName);
 }
 

src/mlpack/bindings/R/mlpack/tests/testthat/test-R_binding.R

@@ -113,6 +113,17 @@ test_that("TestUMatrix", {
   }
 })
 
+# Test a transposed matrix.
+test_that("TestTransMatrix", {
+  x <- matrix(rexp(500, rate = .1), ncol = 5)
+
+  output <- test_r_binding(4.0, 12, "hello",
+                           tmatrix_in=x,
+                           matrix_in=x)
+  # If the binding succeeds, the output double will be 10.
+  expect_true(output$double_out == 10.0)
+})
+
 # Test a column vector input parameter.
 test_that("TestCol", {
   x <- matrix(rexp(100, rate = .1), nrow = 1)

src/mlpack/bindings/R/print_input_processing.hpp

@@ -74,6 +74,15 @@ void PrintInputProcessing(
     util::ParamData& d,
     const typename std::enable_if<arma::is_arma_type<T>::value>::type* = 0)
 {
+  std::string extraTransStr = "";
+  if (d.cppType == "arma::mat")
+  {
+    if (d.noTranspose)
+      extraTransStr = ", FALSE";
+    else
+      extraTransStr = ", TRUE";
+  }
+
   if (!d.required)
   {
     /**
@@ -82,11 +91,21 @@ void PrintInputProcessing(
      *     if (!identical(<param_name>, NA)) {
      *        SetParam<type>(p, "<param_name>", to_matrix(<param_name>))
      *     }
+     *
+     * and if the parameter is an arma::mat, we will get code like
+     *
+     *     if (!identical(<param_name>, NA)) {
+     *        SetParam<type>(p, "<param_name>", to_matrix(<param_name>), TRUE)
+     *     }
+     *
+     * where the final boolean specifies whether the matrix should be
+     * transposed.
      */
     MLPACK_COUT_STREAM << "  if (!identical(" << d.name << ", NA)) {"
         << std::endl;
     MLPACK_COUT_STREAM << "    SetParam" << GetType<T>(d) << "(p, \""
-        << d.name << "\", to_matrix(" << d.name << "))" << std::endl;
+        << d.name << "\", to_matrix(" << d.name << ")" << extraTransStr << ")"
+        << std::endl;
     MLPACK_COUT_STREAM << "  }" << std::endl; // Closing brace.
   }
   else
@@ -95,9 +114,17 @@ void PrintInputProcessing(
      * This gives us code like:
      *
      *     SetParam<type>(p, "<param_name>", to_matrix(<param_name>))
+     *
+     * and if the parameter is an arma::mat, we will get code like
+     *
+     *     SetParam<type>(p, "<param_name>", to_matrix(<param_name>), TRUE)
+     *
+     * where the final boolean specifies whether the matrix should be
+     * transposed.
      */
     MLPACK_COUT_STREAM << "  SetParam" << GetType<T>(d) << "(p, \""
-        << d.name << "\", to_matrix(" << d.name << "))" << std::endl;
+        << d.name << "\", to_matrix(" << d.name << ")" << extraTransStr << ")"
+        << std::endl;
   }
   MLPACK_COUT_STREAM << std::endl; // Extra line is to clear up the code a bit.
 }

src/mlpack/bindings/R/tests/test_r_binding_main.cpp

@@ -41,6 +41,7 @@ PARAM_FLAG("flag1", "Input flag, must be specified.", "f");
 PARAM_FLAG("flag2", "Input flag, must not be specified.", "F");
 PARAM_MATRIX_IN("matrix_in", "Input matrix.", "m");
 PARAM_UMATRIX_IN("umatrix_in", "Input unsigned matrix.", "u");
+PARAM_TMATRIX_IN("tmatrix_in", "Input (transposed) matrix.", "");
 PARAM_COL_IN("col_in", "Input column.", "c");
 PARAM_UCOL_IN("ucol_in", "Input unsigned column.", "");
 PARAM_ROW_IN("row_in", "Input row.", "");
@@ -92,6 +93,30 @@ void BINDING_FUNCTION(util::Params& params, util::Timers& /* timers */)
       params.Get<double>("double_out") = 5.0;
   }
 
+  // If a transposed matrix is specified, check that it is the same as the (now
+  // required) matrix_in parameter.  If the test passes, set `double_out` to 10.
+  if (params.Has("tmatrix_in"))
+  {
+    if (!params.Has("matrix_in"))
+    {
+      throw std::runtime_error("If tmatrix_in is specified, matrix_in must also"
+          " be specified!");
+    }
+
+    arma::mat tmat = params.Get<arma::mat>("tmatrix_in");
+    arma::mat mat = params.Get<arma::mat>("matrix_in");
+
+    if (!arma::approx_equal(tmat.t(), mat, "reldiff", 0.001))
+    {
+      throw std::runtime_error("Transposed tmatrix_in and matrix_in are not "
+          "equal!");
+    }
+    else
+    {
+      params.Get<double>("double_out") = 10.0;
+    }
+  }
+
   // Input matrices should be at least 5 rows; the 5th row will be dropped and
   // the 3rd row will be multiplied by two.
   if (params.Has("matrix_in"))

src/mlpack/bindings/go/CMakeLists.txt

@@ -128,7 +128,7 @@ if (BUILD_GO_SHLIB)
           ${CMAKE_BINARY_DIR}/src/mlpack/bindings/go/src/mlpack.org/v1/mlpack/)
   endforeach()
 
-  if (BUILD_TESTS OR BUILD_GO_SHLIB)
+  if (BUILD_TESTS AND BUILD_GO_SHLIB)
     foreach(test_file ${TEST_SOURCES})
       add_custom_command(TARGET go_copy PRE_BUILD
           COMMAND ${CMAKE_COMMAND} ARGS -E copy_if_different
@@ -278,6 +278,6 @@ endif()
 
 endmacro ()
 
-if (BUILD_TESTS OR BUILD_GO_SHLIB)
+if (BUILD_TESTS AND BUILD_GO_SHLIB)
   add_subdirectory(tests)
 endif ()

src/mlpack/bindings/go/mlpack/arma_util.go

@@ -91,7 +91,7 @@ func (m *mlpackArma) allocArmaPtrMatWithInfo(p *params,
 }
 
 // Passes a Gonum matrix to C by using the underlying data from the Gonum matrix.
-func gonumToArmaMat(p *params, identifier string, m *mat.Dense) {
+func gonumToArmaMat(p *params, identifier string, m *mat.Dense, trans bool) {
   // Get the number of elements in the Armadillo column.
   r, c := m.Dims()
   blas64General := m.RawMatrix()
@@ -100,7 +100,7 @@ func gonumToArmaMat(p *params, identifier string, m *mat.Dense) {
   // Pass pointer of the underlying matrix to mlpack.
   ptr := unsafe.Pointer(&data[0])
   C.mlpackToArmaMat(p.mem, C.CString(identifier), (*C.double)(ptr),
-      C.size_t(c), C.size_t(r))
+      C.size_t(c), C.size_t(r), C.bool(trans))
 }
 
 // Passes a Gonum matrix to C by using the underlying data from the Gonum matrix.

src/mlpack/bindings/go/mlpack/capi/arma_util.cpp

@@ -28,13 +28,18 @@ void mlpackToArmaMat(void* params,
                      const char* identifier,
                      double* mat,
                      const size_t row,
-                     const size_t col)
+                     const size_t col,
+                     bool transpose)
 {
   util::Params& p = *((util::Params*) params);
 
   // Advanced constructor.
   arma::mat m(mat, row, col, false, false);
 
+  // Transpose if necessary.
+  if (transpose)
+    arma::inplace_trans(m);
+
   // Set input parameter with corresponding matrix in IO.
   SetParam(p, identifier, m);
 }

src/mlpack/bindings/go/mlpack/capi/arma_util.h

@@ -28,7 +28,8 @@ void mlpackToArmaMat(void* params,
                      const char* identifier,
                      double* mat,
                      const size_t row,
-                     const size_t col);
+                     const size_t col,
+                     bool transpose);
 
 /**
  * Pass Gonum Dense pointer and wrap an Armadillo mat around it.

src/mlpack/bindings/go/print_input_processing.hpp

@@ -149,12 +149,26 @@ void PrintInputProcessing(
    *
    *  // Detect if the parameter was passed; set if so.
    *  if param.Name != nil {
-   *     gonumToArma<type>(params, "paramName", param.Name)
+   *     gonumToArma<type>(params, "paramName", param.Name, false)
    *     setPassed(params, "paramName")
    *  }
+   *
+   * where the boolean parameter indicates if the matrix needs to be transposed,
+   * and is only included for arma::mat type parameters.
    */
   std::cout << prefix << "// Detect if the parameter was passed; set if so."
             << std::endl;
+
+  // Add extra transpose option, but only for arma::mat types.
+  std::string transStrExtra = "";
+  if (d.cppType == "arma::mat")
+  {
+    if (d.noTranspose)
+      transStrExtra = ", true";
+    else
+      transStrExtra = ", false";
+  }
+
   if (!d.required)
   {
     std::cout << prefix << "if param." << goParamName
@@ -163,7 +177,7 @@ void PrintInputProcessing(
     // Print function call to set the given parameter into the io.
     std::cout << prefix << prefix << "gonumToArma" << GetType<T>(d)
               << "(params, \"" << d.name << "\", param." << goParamName
-              << ")" << std::endl;
+              << transStrExtra << ")" << std::endl;
 
     // Print function call to set the given parameter as passed.
     std::cout << prefix << prefix << "setPassed(params, \"" << d.name << "\")"
@@ -176,7 +190,7 @@ void PrintInputProcessing(
     // Print function call to set the given parameter into the io.
     std::cout << prefix << "gonumToArma" << GetType<T>(d)
               << "(params, \"" << d.name << "\", " << goParamName
-              << ")" << std::endl;
+              << transStrExtra << ")" << std::endl;
 
     // Print function call to set the given parameter as passed.
     std::cout << prefix << "setPassed(params, \"" << d.name << "\")"

src/mlpack/bindings/go/tests/go_binding_test.go

@@ -196,6 +196,31 @@ func TestGonumUMatrix(t *testing.T) {
     }
   }
 }
+
+func TestGonumTransMatrix(t *testing.T) {
+  t.Log("Test transposed matrix input.")
+  x := mat.NewDense(3, 5, []float64{
+    1, 2, 3, 4, 5,
+    6, 7, 8, 9, 10,
+    11, 12, 13, 14, 15,
+  })
+  x2 := mat.NewDense(3, 5, []float64{
+    1, 2, 3, 4, 5,
+    6, 7, 8, 9, 10,
+    11, 12, 13, 14, 15,
+  })
+
+  param := mlpack.TestGoBindingOptions()
+  param.MatrixIn = x
+  param.TmatrixIn = x2
+  d := 4.0
+  i := 12
+  s := "hello"
+  // The binding simply needs to run successfully (without exception) to
+  // succeed.
+  mlpack.TestGoBinding(d, i, s, param)
+}
+
 func TestGonumTransposeRow(t *testing.T) {
   t.Log("Test a column vector input parameter.")
   x := mat.NewDense(1, 9, []float64{

src/mlpack/bindings/go/tests/test_go_binding_main.cpp

@@ -41,6 +41,7 @@ PARAM_FLAG("flag1", "Input flag, must be specified.", "f");
 PARAM_FLAG("flag2", "Input flag, must not be specified.", "F");
 PARAM_MATRIX_IN("matrix_in", "Input matrix.", "m");
 PARAM_UMATRIX_IN("umatrix_in", "Input unsigned matrix.", "u");
+PARAM_TMATRIX_IN("tmatrix_in", "Input matrix (transposed).", "");
 PARAM_COL_IN("col_in", "Input column.", "c");
 PARAM_UCOL_IN("ucol_in", "Input unsigned column.", "");
 PARAM_ROW_IN("row_in", "Input row.", "");
@@ -92,6 +93,26 @@ void BINDING_FUNCTION(util::Params& params, util::Timers& /* timer */)
       params.Get<double>("double_out") = 5.0;
   }
 
+  // If a transposed input matrix is given, it is expected to be the same as the
+  // (now required) input matrix.
+  if (params.Has("tmatrix_in"))
+  {
+    if (!params.Has("matrix_in"))
+    {
+      throw std::runtime_error("If tmatrix_in is specified, matrix_in must be "
+          "specified!");
+    }
+
+    arma::mat tmat = params.Get<arma::mat>("tmatrix_in");
+    arma::mat mat = params.Get<arma::mat>("matrix_in");
+
+    if (!arma::approx_equal(tmat.t(), mat, "reldiff", 0.001))
+    {
+      throw std::runtime_error("Transposed tmatrix_in and matrix_in are not "
+          "equal!");
+    }
+  }
+
   // Input matrices should be at least 5 rows; the 5th row will be dropped and
   // the 3rd row will be multiplied by two.
   if (params.Has("matrix_in"))

src/mlpack/bindings/julia/mlpack/params.jl.in

@@ -47,12 +47,14 @@ function convert_to_1d(in::Array{T, 2})::Array{T, 1} where T
 end
 
 # Utility function to convert to and return a matrix.
-function to_matrix(input, T::Type)
-  if isa(input, Array{T, 1})
+function to_matrix(input, T::Type, transpose::Bool)
+  result = if isa(input, Array{T, 1})
     convert_to_2d(input)
   else
     convert(Array{T, 2}, input)
   end
+
+  return transpose ? convert(Array{T, 2}, result') : result
 end
 
 # Utility function to convert to and return a vector.
@@ -104,9 +106,10 @@ function SetParamMat(params::Ptr{Nothing},
                      paramName::String,
                      paramValue,
                      pointsAsRows::Bool,
+                     transpose::Bool,
                      juliaOwnedMemory::Set{Ptr{Nothing}})
   push!(juliaOwnedMemory, convert(Ptr{Nothing}, Base.pointer(paramValue)))
-  paramMat = to_matrix(paramValue, Float64)
+  paramMat = to_matrix(paramValue, Float64, transpose)
   ccall((:SetParamMat, library), Nothing, (Ptr{Nothing}, Cstring, Ptr{Float64},
       Csize_t, Csize_t, Bool), params, paramName, Base.pointer(paramMat),
       size(paramMat, 1), size(paramMat, 2), pointsAsRows)
@@ -116,9 +119,10 @@ function SetParamUMat(params::Ptr{Nothing},
                       paramName::String,
                       paramValue,
                       pointsAsRows::Bool,
+                      transpose::Bool,
                       juliaOwnedMemory::Set{Ptr{Nothing}})
   push!(juliaOwnedMemory, convert(Ptr{Nothing}, Base.pointer(paramValue)))
-  paramMat = to_matrix(paramValue, Int)
+  paramMat = to_matrix(paramValue, Int, transpose)
 
   # Sanity check.
   if minimum(paramMat) <= 0

src/mlpack/bindings/julia/print_input_processing_impl.hpp

@@ -99,7 +99,12 @@ void PrintInputProcessing(
   else
   {
     matTypeModifier = "Mat";
-    extra = ", points_are_rows";
+
+    // Determine whether we need to transpose the matrix.
+    std::string transStr =
+        (d.noTranspose ? std::string("true") : std::string("false"));
+
+    extra = ", points_are_rows, " + transStr;
   }
 
   // Now print the SetParam call.