RELEASE 0.1.5 (#20)

* Change tuple conversion to always give cell #18

* Workaround for segfault on multiple reuse of wrap np array #19

* Update tests

* Fix nargout bug; now defaults to nargout=0 spinw#147

* Update CITATION.cff, CHANGELOG.md

* Fix get_nlhs() function for nested calls

* Restore get_nlhs from disassembly

* Fix heap error in converter in call_python

* Move py fun wrapper to type_conv from libpymcr main

* Fix euphonic/tobyfit errors

Change call_python to use global dict
Update to work on Python 3.11
Add setdlopenflags(DEEPBIND) for Linux

.github/workflows/main.yml

@@ -192,4 +192,4 @@ jobs:
           name: ${{ matrix.os }}_artifacts.zip
           path: |
             wheelhouse/*
-            *.mex*
+            **/*.mex[awmaci]*64

CHANGELOG.md

@@ -1,3 +1,16 @@
+# [v0.1.5](https://github.com/pace-neutrons/libpymcr/compare/v0.1.4...v0.1.5)
+
+## Bugfixes for PySpinW
+
+Bugfixes for PySpinW identified by users and during the RAL-India workshop.
+
+* Fix a segfault when temporary numpy arrays are re-used multiple times in definition of SpinW objects
+* Change behaviour of tuples and lists. Python tuples now always convert to Matlab cells. Nested lists will convert to Matlab numeric arrays if they are consistent in shape and contain only numbers. This allows Python `([1,2,3], [4,5,6])` to convert to Matlab `{[1 2 3] [4 5 6]}` whereas before it would have converted to Matlab `[1 2 3; 4 5 6]`. Python `[[1,2,3], [4,5,6]]` will still convert to Matlab `[1 2 3; 4 5 6]`.
+* Fix bug where Matlab commands which return zero outputs fail, e.g. `m.axis([0,1,0,1])` due to incorrectly given `nargout` in Matlab.py / call.m
+* New `get_nlhs` algorithm to set `nargout` parameters uses `ast` to better parse cases of nested Matlab calls, like `m.eig(m.rand(3))` but this only works if the full command is on one line; and also does not work for the basic interpreter (but should work in Mantid, Jupyter and Spyder). For those cases, the old `dis` algorithm is used, updated to work with Python 3.11 but does not handle nested calls.
+* Fix bugs in `call_python`. Changed mechanism back to using a global `dict` as direct memory address was failing in some nested calls. `call_python` now creates its own instance of `type_converter` to avoid a heap memory error when using the Python initialized object from the `libpymcr` instance. Re-add `DEEPBIND` library loading for Linux Lapack/BLAS to avoid conflict with Matlab.
+
+
 # [v0.1.4](https://github.com/pace-neutrons/libpymcr/compare/v0.1.3...v0.1.4)
 
 ## Add IPython magics

CITATION.cff

@@ -15,7 +15,7 @@ authors:
     given-names: "Gregory S."
     orcid: https://orcid.org/0000-0002-2787-8054
 title: "libpymcr"
-version: "0.1.4"
+version: "0.1.5"
 date-released: "2023-03-24"
 license: "GPL-3.0-only"
 repository: "https://github.com/pace-neutrons/libpymcr"

CMakeLists.txt

@@ -8,7 +8,7 @@ set(CMAKE_MACOSX_RPATH TRUE)
 set(CMAKE_CXX_STANDARD 11)
 set(CXX_STANDARD_REQUIRED 11)
 
-set(MINIMUM_PYBIND11_VERSION 2.10.0)
+set(MINIMUM_PYBIND11_VERSION 2.10.1)
 set(FETCH_PYBIND11_REPO https://github.com/pybind/pybind11)
 
 if (PYTHON_EXECUTABLE)

libpymcr/Matlab.py

@@ -11,6 +11,18 @@
 _global_matlab_ref = None
 _has_registered_magic = None
 
+# On Linux we need to load the BLAS/LAPACK libraries with the DEEPBIND
+# flag so it doesn't conflict with Matlab's BLAS/LAPACK.
+# This only works if users `import libpymcr` before they import scipy...
+if platform.system() == 'Linux':
+    old_flags = sys.getdlopenflags()
+    sys.setdlopenflags(os.RTLD_NOW | os.RTLD_DEEPBIND)
+    try:
+        import scipy.linalg
+    except ImportError:
+        pass
+    sys.setdlopenflags(old_flags)
+
 
 class _MatlabInstance(object):
     def __init__(self, ctffile, matlab_dir=None, options=None):
@@ -48,13 +60,13 @@ def __getattr__(self, name):
 
     def __call__(self, *args, **kwargs):
         nargout = kwargs.pop('nargout') if 'nargout' in kwargs.keys() else None
-        nreturn = get_nlhs()
+        nreturn = get_nlhs(self._name)
         if nargout is None:
             mnargout, undetermined = self._interface.call('getArgOut', self._name, nargout=2)
             if not undetermined:
-                nargout = max(min(int(mnargout), nreturn), 1)
+                nargout = min(int(mnargout), nreturn)
             else:
-                nargout = max(nreturn, 1)
+                nargout = nreturn
         args += sum(kwargs.items(), ())
         args = unwrap(args, self._interface)
         return wrap(self._interface.call(self._name, *args, nargout=nargout), self._interface)

libpymcr/MatlabProxyObject.py

@@ -38,7 +38,7 @@ def __init__(self, proxy, method):
         self.method = method
 
     def __call__(self, *args, **kwargs):
-        nreturn = get_nlhs()
+        nreturn = get_nlhs(self.method)
         nargout = int(kwargs.pop('nargout') if 'nargout' in kwargs.keys() else nreturn)
         nargout = max(min(nargout, nreturn), 1)
         ifc = self.proxy.interface
@@ -73,7 +73,8 @@ def __init__(self, interface, handle):
         """
         self.__dict__['handle'] = handle
         self.__dict__['interface'] = interface
-        self.__dict__['_is_handle_class'] = self.interface.call('isa', self.handle, 'handle', nargout=1)
+        self.__dict__['_methods'] = []
+        #self.__dict__['_is_handle_class'] = self.interface.call('isa', self.handle, 'handle', nargout=1)
 
         # This cause performance slow downs for large data members and an recursion issue with
         # samples included in sqw object (each sample object is copied to all dependent header "files")
@@ -96,7 +97,9 @@ def _getMethodNames(self):
         Gets methods from a MATLAB object
         :return: list of method names
         """
-        return self.interface.call('methods', self.handle)
+        if not self._methods:
+            self.__dict__['_methods'] = self.interface.call('methods', self.handle)
+        return self._methods
 
     def __getattr__(self, name):
         """Retrieve a value or function from the object.
@@ -115,7 +118,7 @@ def __getattr__(self, name):
             except TypeError:
                 return None
         # if it's a method, wrap it in a functor
-        elif name in self.interface.call('methods', self.handle, nargout=1):
+        elif name in self._methods:
             return matlab_method(self, name)
 
     def __setattr__(self, name, value):

libpymcr/__init__.py

@@ -4,3 +4,5 @@
 from .Matlab import Matlab
 from .MatlabProxyObject import MatlabProxyObject
 from . import utils
+
+_globalFunctionDict = {}

libpymcr/utils.py

@@ -3,17 +3,103 @@
 import glob
 import platform
 import zipfile
-import traceback
+import ast
+import inspect
+import dis
+import linecache
 from pathlib import Path
 
 
-def get_nlhs():
-    caller = traceback.extract_stack()[-3].line
-    retvals = (0, '')
-    if '=' in caller and '(' in caller and caller.index('=') < caller.index('('):
-        return len(caller.split('=')[0].split(','))
-    else:
+OPERATOR_NAMES = {
+    "CALL_FUNCTION", "CALL_FUNCTION_VAR", "CALL_FUNCTION_KW", "CALL_FUNCTION_VAR_KW", "CALL",
+    "UNARY_POSITIVE", "UNARY_NEGATIVE", "UNARY_NOT", "UNARY_CONVERT", "UNARY_INVERT", "GET_ITER",
+    "BINARY_POWER", "BINARY_MULTIPLY", "BINARY_DIVIDE", "BINARY_FLOOR_DIVIDE", "BINARY_TRUE_DIVIDE",
+    "BINARY_MODULO", "BINARY_ADD", "BINARY_SUBTRACT", "BINARY_SUBSCR", "BINARY_LSHIFT",
+    "BINARY_RSHIFT", "BINARY_AND", "BINARY_XOR", "BINARY_OR",
+    "INPLACE_POWER", "INPLACE_MULTIPLY", "INPLACE_DIVIDE", "INPLACE_TRUE_DIVIDE", "INPLACE_FLOOR_DIVIDE",
+    "INPLACE_MODULO", "INPLACE_ADD", "INPLACE_SUBTRACT", "INPLACE_LSHIFT", "INPLACE_RSHIFT",
+    "INPLACE_AND", "INPLACE_XOR", "INPLACE_OR", "COMPARE_OP", "SET_UPDATE", "BUILD_CONST_KEY_MAP",
+    "CALL_FUNCTION_EX", "LOAD_METHOD", "CALL_METHOD", "DICT_MERGE", "DICT_UPDATE", "LIST_EXTEND",
+}
+
+
+def get_nret_from_dis(frame):
+    # Tries to get the number of return values for a function
+    # Code adapted from Mantid/Framework/PythonInterface/mantid/kernel/funcinspect.py
+    ins_stack = []
+    call_fun_locs = {}
+    start_i = 0
+    start_offset = 0
+    last_i = frame.f_lasti
+    for index, ins in enumerate(dis.get_instructions(frame.f_code)):
+        ins_stack.append(ins)
+        if ins.opname in OPERATOR_NAMES:
+            call_fun_locs[start_offset] = start_i
+            start_i = index
+            start_offset = ins.offset
+    call_fun_locs[start_offset] = start_i
+    last_fun_offset = call_fun_locs[last_i]
+    last_i_name = ins_stack[last_fun_offset].opname
+    next_i_name = ins_stack[last_fun_offset + 1].opname
+    if last_i_name == 'DICT_MERGE' and next_i_name in OPERATOR_NAMES:
+        last_fun_offset += 1
+        last_i = ins_stack[last_fun_offset + 1].offset
+    res_i_name = ins_stack[last_fun_offset + 1].opname
+    if res_i_name == 'POP_TOP':
+        return 0
+    elif res_i_name == 'STORE_FAST' or res_i_name == 'STORE_NAME':
         return 1
+    elif res_i_name == 'UNPACK_SEQUENCE':
+        return ins_stack[last_fun_offset + 1].argval
+    elif res_i_name == 'LOAD_FAST' or res_i_name == 'LOAD_NAME':
+        return 1  # Dot-assigment to a member or in a multi-line call
+    elif res_i_name == 'DUP_TOP':
+        raise NotImplementedError('libpymcr does not support multiple assignment')
+    else:
+        return 1  # Probably in a multi-line call
+
+
+def get_nlhs(name):
+    # Tries to get the number of return values for a named (Matlab) function
+    # Assumes that it's called as a method of the `m` or Matlab() object
+
+    def get_branch_of_call(astobj, parent=[]):
+        if isinstance(astobj, ast.Call) and isinstance(astobj.func, ast.Attribute) and astobj.func.attr == name:
+            return astobj
+        for x in ast.iter_child_nodes(astobj):
+            rv = get_branch_of_call(x, parent)
+            if rv:
+                parent.append(astobj)
+                return parent
+        raise SyntaxError('Empty syntax tree')
+
+    def get_nret_from_call(caller):
+        if isinstance(caller, ast.Call):
+            return 1                              # f1(m.<func>())
+        elif isinstance(caller, ast.Assign):
+            targ = caller.targets[0]
+            if isinstance(targ, ast.Tuple):
+                return len(targ.elts)             # x, y = m.<func>()
+            elif isinstance(targ, ast.Name):
+                return 1                          # x = m.<func>()
+        elif isinstance(caller, ast.Expr):
+            return 0                              # m.<func>()
+        elif isinstance(caller, ast.Compare):
+            return 1                              # x == m.<func>()
+        else:
+            return 1
+
+    # First gets the Python line where its called, then convert it to an abstract syntax
+    # tree and parse that to get the branch which leads to this call (in reverse order)
+    # The first element of this branch is the direct caller of this function
+    frame = inspect.currentframe().f_back.f_back
+    call_line = linecache.getline(frame.f_code.co_filename, frame.f_lineno)
+    try:
+        ast_branch = get_branch_of_call(ast.parse(call_line))
+    except SyntaxError:
+        return get_nret_from_dis(frame)
+    else:
+        return get_nret_from_call(ast_branch[0])
 
 
 def get_version_from_ctf(ctffile):

src/CMakeLists.txt

@@ -16,7 +16,7 @@ target_include_directories(call_python PUBLIC ${PYTHON_INCLUDE_DIRS})
 target_include_directories(call_python PUBLIC "${pybind11_SOURCE_DIR}/include")
 ##target_link_libraries(call_python type_converter)
 ## Explicitly does not link with Matlab libraries (we will call them with dlopen etc)
-#set_property(TARGET call_python PROPERTY LINK_LIBRARIES "")
+set_property(TARGET call_python PROPERTY LINK_LIBRARIES "")
 if(NOT ${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
     target_link_libraries(call_python ${PYTHON_LIBRARIES})
 endif()

src/call.m

@@ -46,8 +46,8 @@
 
 function out = unwrap(in_obj)
     out = in_obj;
-    if isstruct(in_obj) && isfield(in_obj, 'func_ptr') && isfield(in_obj, 'converter')
-        out = @(varargin) call('_call_python', [in_obj.func_ptr, in_obj.converter], varargin{:});
+    if isstruct(in_obj) && isfield(in_obj, 'libpymcr_func_ptr')
+        out = @(varargin) call('_call_python', in_obj.libpymcr_func_ptr, varargin{:});
     elseif isa(in_obj, 'containers.Map') && in_obj.isKey('wrapped_oldstyle_class')
         out = in_obj('wrapped_oldstyle_class');
     elseif iscell(in_obj)
@@ -113,12 +113,12 @@
             try
                 n = nargout(name);
             catch
-                n = 1;
+                n = 0;
                 undetermined = true;
             end
         end
     else
-        n = 1;
+        n = 0;
         undetermined = true;
     end
 end

src/call_python.cpp

@@ -12,30 +12,34 @@ namespace py = pybind11;
 class MexFunction : public matlab::mex::Function {
 
     protected:
-        py::tuple convMat2np(ArgumentList inputs, py::handle owner, size_t lastInd, libpymcr::pymat_converter* converter) {
+        libpymcr::pymat_converter *_converter = nullptr;
+        py::tuple convMat2np(ArgumentList inputs, py::handle owner, size_t lastInd) {
             // Note that this function must be called when we have the GIL
             size_t narg = inputs.size() + lastInd;
             py::tuple retval(narg);
             for (size_t idx = 1; idx <= narg; idx++) {
-                retval[idx - 1] = converter->to_python(inputs[idx]);
+                retval[idx - 1] = _converter->to_python(inputs[idx]);
             }
             return retval;
         }
 
     public:
         void operator()(ArgumentList outputs, ArgumentList inputs) {
             matlab::data::ArrayFactory factory;
-            if (inputs.size() < 1 || inputs[0].getNumberOfElements() != 2 ||
-                inputs[0].getType() != matlab::data::ArrayType::UINT64) { // Matlab only supports 64-bit
-                    throw std::runtime_error("First input must be pointers to a Python function and converter.");
+            if (inputs.size() < 1 || inputs[0].getType() != matlab::data::ArrayType::CHAR) {
+                throw std::runtime_error("Input must be reference to a Python function.");
             }
-            uintptr_t key = inputs[0][0];
-            uintptr_t conv_addr = inputs[0][1];
+            matlab::data::CharArray key = inputs[0];
 
             PyGILState_STATE gstate = PyGILState_Ensure();  // GIL{
 
-            libpymcr::pymat_converter* converter = reinterpret_cast<libpymcr::pymat_converter*>(conv_addr);
-            PyObject* fn_ptr = reinterpret_cast<PyObject*>(key);
+            if (_converter == nullptr) {
+                _converter = new libpymcr::pymat_converter(libpymcr::pymat_converter::NumpyConversion::WRAP);
+            }
+
+            py::module pyHoraceFn = py::module::import("libpymcr");
+            py::dict fnDict = pyHoraceFn.attr("_globalFunctionDict");
+            PyObject *fn_ptr = PyDict_GetItemString(fnDict.ptr(), key.toAscii().c_str());
 
             if (PyCallable_Check(fn_ptr)) {
                 PyObject *result;
@@ -44,12 +48,12 @@ class MexFunction : public matlab::mex::Function {
                     try {
                         const matlab::data::StructArray in_struct(inputs[endIdx]);
                         const matlab::data::Array v = in_struct[0][matlab::data::MATLABFieldIdentifier("pyHorace_pyKwArgs")];
-                        PyObject *kwargs = converter->to_python(inputs[endIdx]);
+                        PyObject *kwargs = _converter->to_python(inputs[endIdx]);
                         PyDict_DelItemString(kwargs, "pyHorace_pyKwArgs");
-                        py::tuple arr_in = convMat2np(inputs, fn_ptr, -2, converter);
+                        py::tuple arr_in = convMat2np(inputs, fn_ptr, -2);
                         result = PyObject_Call(fn_ptr, arr_in.ptr(), kwargs);
                     } catch (...) {
-                        py::tuple arr_in = convMat2np(inputs, fn_ptr, -1, converter);
+                        py::tuple arr_in = convMat2np(inputs, fn_ptr, -1);
                         result = PyObject_CallObject(fn_ptr, arr_in.ptr());
                     }
                 } catch (...) {
@@ -61,7 +65,7 @@ class MexFunction : public matlab::mex::Function {
                 }
                 else {
                     try {
-                        outputs[0] = converter->to_matlab(result, Py_REFCNT(result)==1);
+                        outputs[0] = _converter->to_matlab(result, Py_REFCNT(result)==1);
                     } catch (char *e) {
                         Py_DECREF(result);
                         PyGILState_Release(gstate);

src/libpymcr.cpp

@@ -4,36 +4,21 @@
 
 namespace libpymcr {
 
-    matlab::data::Array matlab_env::_conv_to_matlab(PyObject* input) {
-        matlab::data::Array rv;
-        if (PyCallable_Check(input)) {
-            matlab::data::ArrayFactory factory;
-            std::uintptr_t addr = reinterpret_cast<std::uintptr_t>(input);
-            std::uintptr_t conv = reinterpret_cast<std::uintptr_t>(&_converter);
-            rv = factory.createStructArray({1, 1}, std::vector<std::string>({"func_ptr", "converter"}));
-            rv[0][std::string("func_ptr")] = factory.createScalar<uint64_t>(addr);
-            rv[0][std::string("converter")] = factory.createScalar<uint64_t>(conv);
-        } else {
-            rv = _converter.to_matlab(input);
-        }
-        return rv;
-    }
-
     size_t matlab_env::_parse_inputs(std::vector<matlab::data::Array>& m_args,
                          py::args py_args,
                          py::kwargs& py_kwargs) {
         matlab::data::ArrayFactory factory;
         size_t nargout = 1;
         for (auto item: py_args) {
-            m_args.push_back(_conv_to_matlab(item.ptr()));
+            m_args.push_back(_converter.to_matlab(item.ptr()));
         }
         for (auto item: py_kwargs) {
             std::string key(py::str(item.first));
             if (key.find("nargout") == 0) {
                 nargout = item.second.cast<size_t>();
             } else {
                 m_args.push_back(factory.createCharArray(std::string(py::str(item.first))));
-                m_args.push_back(_conv_to_matlab(item.second.ptr()));
+                m_args.push_back(_converter.to_matlab(item.second.ptr()));
             }
         }
         return nargout;

src/libpymcr.hpp

@@ -21,7 +21,6 @@ namespace libpymcr {
         std::shared_ptr<StreamBuffer> _m_output_buf = std::static_pointer_cast<StreamBuffer>(_m_output);
         std::shared_ptr<StreamBuffer> _m_error_buf = std::static_pointer_cast<StreamBuffer>(_m_error);
         pymat_converter _converter;
-        matlab::data::Array _conv_to_matlab(PyObject* input);
         size_t _parse_inputs(std::vector<matlab::data::Array>& m_args, py::args py_args, py::kwargs& py_kwargs);
     public:
         py::object feval(const std::u16string &funcname, py::args args, py::kwargs& kwargs);