bastibe / transplant

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

Matlab package is not found when there are directories with the same name #81

Closed dlaidig closed 4 years ago

dlaidig commented 4 years ago

After spending some time trying to call a Matlab function in a package and wondering why I always got an UndefinedFunction error, I finally realized that this was actually caused by the current working directory having the same name as the package.

The same thing seems to happen if the current working directory contains another directory with the same name as the package. I didn't check further to see if this also applies to any directory in Matlab's path.

In Matlab.__getattr__, there is a very specific check for detecting packages: if not (err.identifier == 'TRANSPLANT:novariable' and packagedict):. When the error appears, err.identifier was MATLAB:UndefinedFunction instead of TRANSPLANT:novariable.

Here is a minimum example that should make it possible to reproduce the bug just by pasting the commands:

% mkdir -p transplanttest/projectname/matlab/+projectname/
% echo -e "function []=foobar()\ndisp(42)" > transplanttest/projectname/matlab/+projectname/foobar.m
% python3 -c "import transplant; m=transplant.Matlab(); m.addpath('transplanttest/projectname/matlab'); m.projectname.foobar()"
...
    42
% cd transplanttest
% python3 -c "import transplant; m=transplant.Matlab(); m.addpath('projectname/matlab'); m.projectname.foobar()"
...
transplant.transplant_master.TransplantError: Undefined function or variable 'projectname'. (MATLAB:UndefinedFunction)
...
% cd projectname
% python3 -c "import transplant; m=transplant.Matlab(); m.addpath('matlab'); m.projectname.foobar()"
...
transplant.transplant_master.TransplantError: Undefined function or variable 'projectname'. (MATLAB:UndefinedFunction)
...
% cd matlab
% python3 -c "import transplant; m=transplant.Matlab(); m.projectname.foobar()"
...
    42
bastibe commented 4 years ago

Thank you for your bug report.

If I understand this issue correctly, there is a bug in get_global that produces an error other than TRANSPLANT:novariable. As far as I can tell this means that either which('projectname') or exist('projectname') produced an error in Matlab.

If true, that is a bug in Matlab, not Transplant. (Am I understanding this correctly?)

That said, I'd be grateful for a pull request that fixes this issue, if it can be done on the Matlab side, and without adding a large performance overhead.

dlaidig commented 4 years ago

Thanks for the pointers. From a quick look, it seems like in the failing cases, exist('projectname') returns 7 ("name is a folder") instead of 0.

If I find the time for it, I will try to investigate further and create a pull request.

bastibe commented 4 years ago

That would be greatly appreciated.

dlaidig commented 4 years ago

I did spend some time looking into it. The good news it that a simple

-                    existance = existance | any(which(msg('name')));
+                    existance = ~any(existance == [0, 7]) | any(which(msg('name')));

fixes the bug for me.

The bad news is that I am quite irritated by the next few lines of code:

existance = evalin('base', ['exist(''' msg('name') ''')']);

returns an number between 0 and 8, then

existance = existance | any(which(msg('name')));

turns this into a logical value, i.e. it is either 0 or 1. A few lines later

elseif any(existance == [2, 3, 5, 6]) | any(which(msg('name')))

is checked, and the first part can never be true as existance is always 0 or 1.

I am not sure if I am missing something or the code is indeed broken. I feel like applying my fix without fixing the rest of this is not the right approach but I don't understand the transplant internals enough for a proper fix.

bastibe commented 4 years ago

I wish I could help you. But at the moment, I am stuck at home, without access to Matlab.

But if that fix works, and the rest of the test suite still runs, I would assume it is good, and I would gratefully accept a pull request.

dlaidig commented 4 years ago

any news on this?

bastibe commented 4 years ago

I am terribly sorry for the late response. Your PR somehow fell off my radar. Again, my apologies.

Thank you very much for dealing with the issue quickly and efficiently, though!

dlaidig commented 4 years ago

Slightly off-topic here, but do you have plans to release a new version that includes the fix at some point? Being able to install a version that includes the fix from PyPI would make things easier for me.

bastibe commented 4 years ago

Sorry about that. I'll try to do it tomorrow morning.

dlaidig commented 3 years ago

Thanks! :) Any update on this?

bastibe commented 3 years ago

Sorry about that. I had an accident that all made desk work all but impossible.

But I just uploaded the release. Thank you for your patience.

dlaidig commented 3 years ago

Thank you!