From 6bf7ea23cfe732eeba5ec68ce86a1d32fe852ea6 Mon Sep 17 00:00:00 2001 From: Duy-Nguyen Ta Date: Wed, 15 Mar 2017 13:47:11 -0400 Subject: [PATCH] convert numpy input params to dtype float and order 'F' automatically using numpy.astype(...). No copy if the params are already in the correct dtype and storage order. For a function f(Matrix A, Matrix B), simply wrapping it to pyx as f(A.astype(float, order='F', copy=False), B.astype(float, order='F', copy=False)) won't work. It produces a strange side-effect that the content of A is overwritten by B and the two inputs are the same (data address) inside the function! This is because Cython decreases the ref count for the temporary variable resulted from A.astype(...) before generates the wrap for B.astype(...). Hence, the A.astype temp var is probably reused for B.astype, and they were pointing to the same data address. For that reason, we have to go a longer route and wrap it as: A = A.astype(float, order='F', copy=False) B = B.astype(float, order='F', copy=False) f(A, B) For future ref., here is a sample of the wrongly generated code that wraps the JacobianFactor constructor: Jacobian(Key i1, Matrix A1, Key i2, Matrix A2, Vector b, noiseModel::Diagonal model) Wrongly wrapped pyx code: self.shared_CJacobianFactor_ = shared_ptr[CJacobianFactor](new CJacobianFactor(i1, (Map[MatrixXd](A1.astype(float, order='F',copy=False)), i2, (Map[MatrixXd](A2.astype(float, order='F', copy=False)), (Map[VectorXd](b.astype(float, order='F', copy=False))), model.shared_CnoiseModel_Diagonal_)) The problematic Cython generated CPP code with a comment on the problematic line: ///////////////////////////////////////// // WRONG VERSION ///////////////////////////////////////// __pyx_t_12 = __Pyx_PyObject_GetAttrStr(((PyObject *)__pyx_v_A1), __pyx_n_s_astype); if (unlikely(!__pyx_t_12)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_12); __pyx_t_4 = PyTuple_New(1); if (unlikely(!__pyx_t_4)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_4); __Pyx_INCREF(((PyObject *)(&PyFloat_Type))); __Pyx_GIVEREF(((PyObject *)(&PyFloat_Type))); PyTuple_SET_ITEM(__pyx_t_4, 0, ((PyObject *)(&PyFloat_Type))); __pyx_t_5 = PyDict_New(); if (unlikely(!__pyx_t_5)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_5); if (PyDict_SetItem(__pyx_t_5, __pyx_n_s_order, __pyx_n_s_F) < 0) __PYX_ERR(0, 2107, __pyx_L1_error) if (PyDict_SetItem(__pyx_t_5, __pyx_n_s_copy, Py_False) < 0) __PYX_ERR(0, 2107, __pyx_L1_error) __pyx_t_13 = __Pyx_PyObject_Call(__pyx_t_12, __pyx_t_4, __pyx_t_5); if (unlikely(!__pyx_t_13)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_13); __Pyx_DECREF(__pyx_t_12); __pyx_t_12 = 0; __Pyx_DECREF(__pyx_t_4); __pyx_t_4 = 0; __Pyx_DECREF(__pyx_t_5); __pyx_t_5 = 0; if (!(likely(((__pyx_t_13) == Py_None) || likely(__Pyx_TypeTest(__pyx_t_13, __pyx_ptype_5numpy_ndarray))))) __PYX_ERR(0, 2107, __pyx_L1_error) try { __pyx_t_14 = eigency::Map (((PyArrayObject *)__pyx_t_13)); } catch(...) { __Pyx_CppExn2PyErr(); __PYX_ERR(0, 2107, __pyx_L1_error) } /////////////////////////////////////////////// __Pyx_DECREF(__pyx_t_13); __pyx_t_13 = 0; //<------- Problematic line!!! Killing this will result in the correct result! /////////////////////////////////////////////// __pyx_t_13 = __Pyx_PyObject_GetAttrStr(((PyObject *)__pyx_v_A2), __pyx_n_s_astype); if (unlikely(!__pyx_t_13)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_13); __pyx_t_5 = PyTuple_New(1); if (unlikely(!__pyx_t_5)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_5); __Pyx_INCREF(((PyObject *)(&PyFloat_Type))); __Pyx_GIVEREF(((PyObject *)(&PyFloat_Type))); PyTuple_SET_ITEM(__pyx_t_5, 0, ((PyObject *)(&PyFloat_Type))); __pyx_t_4 = PyDict_New(); if (unlikely(!__pyx_t_4)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_4); if (PyDict_SetItem(__pyx_t_4, __pyx_n_s_order, __pyx_n_s_F) < 0) __PYX_ERR(0, 2107, __pyx_L1_error) if (PyDict_SetItem(__pyx_t_4, __pyx_n_s_copy, Py_False) < 0) __PYX_ERR(0, 2107, __pyx_L1_error) __pyx_t_12 = __Pyx_PyObject_Call(__pyx_t_13, __pyx_t_5, __pyx_t_4); if (unlikely(!__pyx_t_12)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_12); __Pyx_DECREF(__pyx_t_13); __pyx_t_13 = 0; __Pyx_DECREF(__pyx_t_5); __pyx_t_5 = 0; __Pyx_DECREF(__pyx_t_4); __pyx_t_4 = 0; if (!(likely(((__pyx_t_12) == Py_None) || likely(__Pyx_TypeTest(__pyx_t_12, __pyx_ptype_5numpy_ndarray))))) __PYX_ERR(0, 2107, __pyx_L1_error) try { __pyx_t_15 = eigency::Map (((PyArrayObject *)__pyx_t_12)); } catch(...) { __Pyx_CppExn2PyErr(); __PYX_ERR(0, 2107, __pyx_L1_error) } __Pyx_DECREF(__pyx_t_12); __pyx_t_12 = 0; __pyx_t_12 = __Pyx_PyObject_GetAttrStr(((PyObject *)__pyx_v_b), __pyx_n_s_astype); if (unlikely(!__pyx_t_12)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_12); __pyx_t_4 = PyTuple_New(1); if (unlikely(!__pyx_t_4)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_4); __Pyx_INCREF(((PyObject *)(&PyFloat_Type))); __Pyx_GIVEREF(((PyObject *)(&PyFloat_Type))); PyTuple_SET_ITEM(__pyx_t_4, 0, ((PyObject *)(&PyFloat_Type))); __pyx_t_5 = PyDict_New(); if (unlikely(!__pyx_t_5)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_5); if (PyDict_SetItem(__pyx_t_5, __pyx_n_s_order, __pyx_n_s_F) < 0) __PYX_ERR(0, 2107, __pyx_L1_error) if (PyDict_SetItem(__pyx_t_5, __pyx_n_s_copy, Py_False) < 0) __PYX_ERR(0, 2107, __pyx_L1_error) __pyx_t_13 = __Pyx_PyObject_Call(__pyx_t_12, __pyx_t_4, __pyx_t_5); if (unlikely(!__pyx_t_13)) __PYX_ERR(0, 2107, __pyx_L1_error) __Pyx_GOTREF(__pyx_t_13); __Pyx_DECREF(__pyx_t_12); __pyx_t_12 = 0; __Pyx_DECREF(__pyx_t_4); __pyx_t_4 = 0; __Pyx_DECREF(__pyx_t_5); __pyx_t_5 = 0; if (!(likely(((__pyx_t_13) == Py_None) || likely(__Pyx_TypeTest(__pyx_t_13, __pyx_ptype_5numpy_ndarray))))) __PYX_ERR(0, 2107, __pyx_L1_error) try { __pyx_t_16 = eigency::Map (((PyArrayObject *)__pyx_t_13)); } catch(...) { __Pyx_CppExn2PyErr(); __PYX_ERR(0, 2107, __pyx_L1_error) } __Pyx_DECREF(__pyx_t_13); __pyx_t_13 = 0; try { __pyx_t_17 = new gtsam::JacobianFactor(__pyx_v_i1, ((Eigen::MatrixXd)__pyx_t_14), __pyx_v_i2, ((Eigen::MatrixXd)__pyx_t_15), ((Eigen::VectorXd)__pyx_t_16), __pyx_v_model->shared_CnoiseModel_Diagonal_); } catch(...) { __Pyx_CppExn2PyErr(); __PYX_ERR(0, 2107, __pyx_L1_error) } __pyx_v_self->shared_CJacobianFactor_ = boost::shared_ptr (__pyx_t_17); --- wrap/Argument.cpp | 17 ++++++++++++++++- wrap/Argument.h | 2 ++ wrap/Constructor.cpp | 1 + wrap/GlobalFunction.cpp | 2 ++ wrap/Method.cpp | 2 ++ wrap/StaticMethod.cpp | 2 ++ 6 files changed, 25 insertions(+), 1 deletion(-) diff --git a/wrap/Argument.cpp b/wrap/Argument.cpp index b36094f60..1077e3df6 100644 --- a/wrap/Argument.cpp +++ b/wrap/Argument.cpp @@ -130,6 +130,13 @@ void Argument::emit_cython_pyx(FileWriter& file) const { file.oss << type.pyxArgumentType() << " " << name; } +/* ************************************************************************* */ +std::string Argument::pyx_convertEigenTypeAndStorageOrder() const { + if (!type.isEigen()) + return ""; + return name + " = " + name + ".astype(float, order=\'F\', copy=False)"; +} + /* ************************************************************************* */ std::string Argument::pyx_asParam() const { string cythonType = type.pxdClassName(); @@ -142,7 +149,7 @@ std::string Argument::pyx_asParam() const { } else { cythonVar = name; } - return cythonVar; + return cythonVar; } /* ************************************************************************* */ @@ -242,6 +249,14 @@ void ArgumentList::emit_cython_pyx(FileWriter& file) const { } } +/* ************************************************************************* */ +std::string ArgumentList::pyx_convertEigenTypeAndStorageOrder(const std::string& indent) const { + string ret; + for (size_t j = 0; j < size(); ++j) + ret += indent + at(j).pyx_convertEigenTypeAndStorageOrder() + "\n"; + return ret; +} + /* ************************************************************************* */ std::string ArgumentList::pyx_asParams() const { string ret; diff --git a/wrap/Argument.h b/wrap/Argument.h index 5cc5f7322..c06e4f797 100644 --- a/wrap/Argument.h +++ b/wrap/Argument.h @@ -73,6 +73,7 @@ struct Argument { const std::vector& templateArgs) const; void emit_cython_pyx(FileWriter& file) const; std::string pyx_asParam() const; + std::string pyx_convertEigenTypeAndStorageOrder() const; friend std::ostream& operator<<(std::ostream& os, const Argument& arg) { os << (arg.is_const ? "const " : "") << arg.type << (arg.is_ptr ? "*" : "") @@ -131,6 +132,7 @@ struct ArgumentList: public std::vector { std::string pyx_asParams() const; std::string pyx_paramsList() const; std::string pyx_castParamsToPythonType() const; + std::string pyx_convertEigenTypeAndStorageOrder(const std::string& indent) const; /** * emit checking arguments to MATLAB proxy diff --git a/wrap/Constructor.cpp b/wrap/Constructor.cpp index 69f63519b..b2ebbce4c 100644 --- a/wrap/Constructor.cpp +++ b/wrap/Constructor.cpp @@ -148,6 +148,7 @@ void Constructor::emit_cython_pyx(FileWriter& pyxFile, const Class& cls) const { pyxFile.oss << " def " + cls.pyxClassName() + "_" + to_string(i) + "(self, *args, **kwargs):\n"; pyxFile.oss << pyx_resolveOverloadParams(args, true); + pyxFile.oss << argumentList(i).pyx_convertEigenTypeAndStorageOrder(" "); pyxFile.oss << " self." << cls.shared_pxd_obj_in_pyx() << " = " << cls.shared_pxd_class_in_pyx() << "(new " << cls.pxd_class_in_pyx() diff --git a/wrap/GlobalFunction.cpp b/wrap/GlobalFunction.cpp index 62a652405..ff5c32d65 100644 --- a/wrap/GlobalFunction.cpp +++ b/wrap/GlobalFunction.cpp @@ -164,6 +164,7 @@ void GlobalFunction::emit_cython_pyx_no_overload(FileWriter& file) const { file.oss << "):\n"; /// Call cython corresponding function and return + file.oss << argumentList(0).pyx_convertEigenTypeAndStorageOrder(" "); string ret = pyx_functionCall("", pxdName(), 0); if (!returnVals_[0].isVoid()) { file.oss << " cdef " << returnVals_[0].pyx_returnType() @@ -200,6 +201,7 @@ void GlobalFunction::emit_cython_pyx(FileWriter& file) const { file.oss << pyx_resolveOverloadParams(args, false, 1); // lazy: always return None even if it's a void function /// Call cython corresponding function + file.oss << argumentList(i).pyx_convertEigenTypeAndStorageOrder(" "); string ret = pyx_functionCall("", pxdName(), i); if (!returnVals_[i].isVoid()) { file.oss << " cdef " << returnVals_[i].pyx_returnType() diff --git a/wrap/Method.cpp b/wrap/Method.cpp index 306ea986d..9670fb090 100644 --- a/wrap/Method.cpp +++ b/wrap/Method.cpp @@ -115,6 +115,7 @@ void Method::emit_cython_pyx_no_overload(FileWriter& file, file.oss << "):\n"; /// Call cython corresponding function and return + file.oss << argumentList(0).pyx_convertEigenTypeAndStorageOrder(" "); string caller = "self." + cls.shared_pxd_obj_in_pyx() + ".get()"; string ret = pyx_functionCall(caller, funcName, 0); if (!returnVals_[0].isVoid()) { @@ -158,6 +159,7 @@ void Method::emit_cython_pyx(FileWriter& file, const Class& cls) const { file.oss << pyx_resolveOverloadParams(args, false); // lazy: always return None even if it's a void function /// Call cython corresponding function + file.oss << argumentList(i).pyx_convertEigenTypeAndStorageOrder(" "); string caller = "self." + cls.shared_pxd_obj_in_pyx() + ".get()"; string ret = pyx_functionCall(caller, funcName, i); diff --git a/wrap/StaticMethod.cpp b/wrap/StaticMethod.cpp index e2a5a31ff..e60228199 100644 --- a/wrap/StaticMethod.cpp +++ b/wrap/StaticMethod.cpp @@ -79,6 +79,7 @@ void StaticMethod::emit_cython_pyx_no_overload(FileWriter& file, file.oss << "):\n"; /// Call cython corresponding function and return + file.oss << argumentList(0).pyx_convertEigenTypeAndStorageOrder(" "); string ret = pyx_functionCall(cls.pxd_class_in_pyx(), name_, 0); file.oss << " "; if (!returnVals_[0].isVoid()) { @@ -117,6 +118,7 @@ void StaticMethod::emit_cython_pyx(FileWriter& file, const Class& cls) const { file.oss << pyx_resolveOverloadParams(args, false); // lazy: always return None even if it's a void function /// Call cython corresponding function and return + file.oss << argumentList(i).pyx_convertEigenTypeAndStorageOrder(" "); string ret = pyx_functionCall(cls.pxd_class_in_pyx(), pxdFuncName, i); if (!returnVals_[i].isVoid()) { file.oss << " cdef " << returnVals_[i].pyx_returnType()