From ce655c172152ca9f7c0afaff0df08070d0f7d6ae Mon Sep 17 00:00:00 2001 From: Ryan Curtin Date: Thu, 1 Dec 2022 09:47:17 -0500 Subject: [PATCH] Fix TMATRIX handling for Python by using doTranspose member. --- src/mlpack/bindings/python/mlpack/io.pxd | 1 + src/mlpack/bindings/python/mlpack/io_util.hpp | 29 ++++++++++++++++++- .../python/print_input_processing.hpp | 15 ++++++---- .../python/tests/test_python_binding.py | 29 +++++++++++++++++++ .../python/tests/test_python_binding_main.cpp | 22 ++++++++++++++ 5 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/mlpack/bindings/python/mlpack/io.pxd b/src/mlpack/bindings/python/mlpack/io.pxd index 99159561a9..f94785d565 100644 --- a/src/mlpack/bindings/python/mlpack/io.pxd +++ b/src/mlpack/bindings/python/mlpack/io.pxd @@ -25,6 +25,7 @@ cdef extern from "" namespace "mlpack" nogil: cdef extern from "" \ namespace "mlpack::util" nogil: void SetParam[T](Params, string, T&) nogil except + + void SetParam[T](Params, string, T&, bool) nogil except + void SetParamPtr[T](Params, string, T*, bool) nogil except + void SetParamWithInfo[T](Params, string, T&, const bool*) nogil except + (T*) GetParamPtr[T](Params, string) nogil except + diff --git a/src/mlpack/bindings/python/mlpack/io_util.hpp b/src/mlpack/bindings/python/mlpack/io_util.hpp index 9966924cd0..8c2ad26da0 100644 --- a/src/mlpack/bindings/python/mlpack/io_util.hpp +++ b/src/mlpack/bindings/python/mlpack/io_util.hpp @@ -19,20 +19,47 @@ namespace mlpack { namespace util { +// Utility functions to correctly handle transposed Armadillo matrices. +template +inline void TransposeIfNeeded( + const std::string& identifier, + T& value, + bool transpose) +{ + // No transpose needed for non-matrices. + return; +} + +inline void TransposeIfNeeded( + const std::string& identifier, + arma::mat& value, + bool transpose) +{ + if (transpose) + { + arma::inplace_trans(value); + } +} + /** * Set the parameter to the given value. * * This function exists to work around Cython's lack of support for lvalue * references. * + * @param params Parameters object to use. * @param identifier Name of parameter. * @param value Value to set parameter to. + * @param transpose If true, and if T is a matrix type, the matrix will be + * transposed in-place. */ template inline void SetParam(util::Params& params, const std::string& identifier, - T& value) + T& value, + bool transpose = false) { + TransposeIfNeeded(identifier, value, transpose); params.Get(identifier) = std::move(value); } diff --git a/src/mlpack/bindings/python/print_input_processing.hpp b/src/mlpack/bindings/python/print_input_processing.hpp index 584b6a31d2..2cdd01fc4b 100644 --- a/src/mlpack/bindings/python/print_input_processing.hpp +++ b/src/mlpack/bindings/python/print_input_processing.hpp @@ -271,13 +271,18 @@ void PrintInputProcessing( * param_name_tuple[0].shape = (param_name_tuple[0].size,) * param_name_mat = arma_numpy.numpy_to_mat_s(param_name_tuple[0], * param_name_tuple[1]) - * SetParam[mat](p, \ 'param_name', dereference(param_name_mat)) + * SetParam[mat](p, \ 'param_name', dereference(param_name_mat), True) * p.SetPassed(\ 'param_name') * + * The value of the final boolean passed to SetParam is determined by whether + * the matrix is transposed or not. That boolean is omitted if the parameter + * is a row or column. */ std::cout << prefix << "# Detect if the parameter was passed; set if so." << std::endl; std::string name = GetValidName(d.name); + std::string transStr = + (d.noTranspose ? std::string("True") : std::string("False")); if (!d.required) { @@ -299,7 +304,7 @@ void PrintInputProcessing( << "_tuple[0], " << name << "_tuple[1])" << std::endl; std::cout << prefix << " SetParam[" << GetCythonType(d) << "](p, '" << d.name << "', dereference(" - << name << "_mat))"<< std::endl; + << name << "_mat))" << std::endl; std::cout << prefix << " p.SetPassed( '" << d.name << "')" << std::endl; std::cout << prefix << " del " << name << "_mat" << std::endl; @@ -319,7 +324,7 @@ void PrintInputProcessing( << "_tuple[0], " << name << "_tuple[1])" << std::endl; std::cout << prefix << " SetParam[" << GetCythonType(d) << "](p, '" << d.name << "', dereference(" - << name << "_mat))"<< std::endl; + << name << "_mat), " << transStr << ")" << std::endl; std::cout << prefix << " p.SetPassed( '" << d.name << "')" << std::endl; std::cout << prefix << " del " << name << "_mat" << std::endl; @@ -343,7 +348,7 @@ void PrintInputProcessing( << "_tuple[0], " << name << "_tuple[1])" << std::endl; std::cout << prefix << "SetParam[" << GetCythonType(d) << "](p, '" << d.name << "', dereference(" - << name << "_mat))"<< std::endl; + << name << "_mat))" << std::endl; std::cout << prefix << "p.SetPassed( '" << d.name << "')" << std::endl; std::cout << prefix << "del " << name << "_mat" << std::endl; @@ -362,7 +367,7 @@ void PrintInputProcessing( << "_tuple[0], " << name << "_tuple[1])" << std::endl; std::cout << prefix << "SetParam[" << GetCythonType(d) << "](p, '" << d.name << "', dereference(" << name - << "_mat))" << std::endl; + << "_mat), " << transStr << ")" << std::endl; std::cout << prefix << "p.SetPassed( '" << d.name << "')" << std::endl; std::cout << prefix << "del " << name << "_mat" << std::endl; diff --git a/src/mlpack/bindings/python/tests/test_python_binding.py b/src/mlpack/bindings/python/tests/test_python_binding.py index dd67aed974..bfa21810d1 100644 --- a/src/mlpack/bindings/python/tests/test_python_binding.py +++ b/src/mlpack/bindings/python/tests/test_python_binding.py @@ -557,6 +557,35 @@ class TestPythonBinding(unittest.TestCase): self.assertEqual(output['umatrix_out'][2, 2], 26) self.assertEqual(output['umatrix_out'][2, 3], 14) + def testTransMatrix(self): + """ + Test that we can correctly pass a matrix in that's specified with the + PARAM_TMATRIX_IN() macro, and it is correctly transposed. + """ + x = np.random.rand(20, 10) + test_python_binding(string_in='hello', + int_in=12, + double_in=4.0, + mat_req_in=[[1.0]], + col_req_in=[1.0], + matrix_in=x, + tmatrix_in=x) + + def testTransMatrixForceCopy(self): + """ + The same test as above, but we force copies. + """ + x = np.random.rand(20, 10) + xt = copy.deepcopy(np.transpose(x)) + test_python_binding(string_in='hello', + int_in=12, + double_in=4.0, + mat_req_in=[[1.0]], + col_req_in=[1.0], + matrix_in=x, + tmatrix_in=x, + copy_all_inputs=True) + def testCol(self): """ Test a column vector input parameter. diff --git a/src/mlpack/bindings/python/tests/test_python_binding_main.cpp b/src/mlpack/bindings/python/tests/test_python_binding_main.cpp index 9bc4720dc8..ce17cf217e 100644 --- a/src/mlpack/bindings/python/tests/test_python_binding_main.cpp +++ b/src/mlpack/bindings/python/tests/test_python_binding_main.cpp @@ -46,6 +46,7 @@ PARAM_FLAG("flag2", "Input flag, must not be specified.", "F"); PARAM_MATRIX_IN("matrix_in", "Input matrix.", "m"); PARAM_MATRIX_IN("smatrix_in", "Input matrix.", ""); 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.", ""); @@ -114,6 +115,27 @@ void BINDING_FUNCTION(util::Params& params, util::Timers& /* timer */) "single element!"); } + // If a transposed input matrix is given, an input matrix should also be + // given, and one should be the transpose of the other. + 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("tmatrix_in"); + std::cerr << "tmat has size " << tmat.n_rows << " x " << tmat.n_cols << "\n"; + arma::mat mat = params.Get("matrix_in"); + std::cerr << "mat has size " << mat.n_rows << " x " << mat.n_cols << "\n"; + + if (!arma::approx_equal(tmat.t(), mat, "reldiff", 0.001)) + { + throw std::runtime_error("tmatrix_in transposed not equal to matrix_in!"); + } + } + // 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"))