bastibe / transplant

Transplant is an easy way of calling Matlab from Python
https://transplant.readthedocs.io
Other
110 stars 26 forks source link

Logical array (matrix) to numpy.ndarray with type bool #84

Closed iled closed 4 years ago

iled commented 4 years ago

First things first: huge thank you for this great work and for sharing it!

I think I have came upon a bug, or perhaps a missing feature.

In my use case, I have an m-file with a function that returns a logical 2-D array. Here is a simple example:

% get_logical.m
function l = get_logical(n)
% generate a random logical matrix of size n x n
l = logical(randi(2, n) - 1);

When I call this function through transplant, I get a list of lists.

>>> m = transplant.Matlab()
>>> m.get_logical(3)
[[True, True, False], [False, False, False], [True, True, True]]

I am not sure if this is the desired behavior. I saw #3 and it was not clear to me what was the intent. For my particular case, this is a drawback. If I call get_logical(10000), this gets very slow. Perhaps that is expectable when dealing with a large list of large lists.

I tried to work around this on transplant_remote.m: https://github.com/iled/transplant/commit/928dc7ceafdffd63fb66153da7a9f9780563ec9b

Now I get:

m.get_logical(3)
array([[ True,  True, False],
       [ True,  True,  True],
       [False, False,  True]])

Please let me know if this is something that would be welcomed on a PR, or, otherwise, if there is a better way to get a ndarray in a case like this.

As it is right now, my modification fails 1 test out of the 13. On test_empty_sparse_matrices, the first assert blows up with a TypeError: a bytes-like object is required, not 'int'. I could not figure out why, so in case of a PR, some other changes are needed.

iled commented 4 years ago

All 13 tests passing now: https://github.com/iled/transplant/commit/d835600cedb1caceaf4eb176aa7c5c1e7fe74bdf I had an error that was trying to encode a single logical value as a matrix.

bastibe commented 4 years ago

Thank you very much for your analysis and bug fix. Your fix looks good to me, my code seems obviously wrong.

Would you like to create a pull request, and add a test case for logical arrays?

iled commented 4 years ago

Sure, I am glad this seems correct! I will add a test case later tonight and then send a PR.

iled commented 4 years ago

Sorry, I didn't have time earlier to submit the PR.

I have added 2 tests, one to put a logical matrix, and another to get. Both of these tests pass, but they failed as of 9a31396f3377f7284f630c8d044940b7df27d85b (I am running behind the 2 latest commits, but I believe that wouldn't make a difference).

I am closing this issue, please let me know if there is any other change needed!