-
Notifications
You must be signed in to change notification settings - Fork 476
Conversation
@@ -11,6 +11,10 @@ | |||
set to the root of your Python installation. If your Python installation | |||
does not contain debug libraries, debug build will not work. --> | |||
<PythonSupport>false</PythonSupport> | |||
<!-- NOTE: If Matlab support is enabled, MatlabDir (below) needs to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Misalignment] Could you please convert tabs to spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll do that in a moment.
Can you add one small section on README.md on how to use MatLab wrapper? Please squash all commits into one, once you are done addressing feedback from PR. Thanks for helping out! |
@pavlejosipovic Sure, no problem. I'll add the section on README.md once we clarify all the issues with this PR. |
<Error Condition="!Exists('..\..\..\NugetPackages\boost_filesystem-vc120.1.59.0.0\build\native\boost_filesystem-vc120.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\NugetPackages\boost_filesystem-vc120.1.59.0.0\build\native\boost_filesystem-vc120.targets'))" /> | ||
<Error Condition="!Exists('..\..\..\NugetPackages\boost_system-vc120.1.59.0.0\build\native\boost_system-vc120.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\NugetPackages\boost_system-vc120.1.59.0.0\build\native\boost_system-vc120.targets'))" /> | ||
<Error Condition="!Exists('..\..\..\NugetPackages\boost_thread-vc120.1.59.0.0\build\native\boost_thread-vc120.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\NugetPackages\boost_thread-vc120.1.59.0.0\build\native\boost_thread-vc120.targets'))" /> | ||
<Error Condition="!Exists('..\..\..\NugetPackages\boost_python2.7-vc120.1.59.0.0\build\native\boost_python-vc120.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\NugetPackages\boost_python2.7-vc120.1.59.0.0\build\native\boost_python-vc120.targets'))" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that boost_python is only necessary for python project, you should be able to remove it from this project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I didn't realize this dependence was there too. I removed it in from my local copy (which I'll upload in a moment). Then, just in case, I tested to compile everything with both PythonSupport and MatlabSupport activated, but matcaffe still needs to link boost_python, if so. I guess it is because a dependence uses it and forces matcaffe to link it. Thus, it seems we still need to deactivate PythonSupport if we want to compile matcaffe.
By the way, this time I tested the mex file in Matlab 2015b and it also works. So, up to now the tested Matlab versions have been: 2014b, 2015a and 2015b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added by me... Because I had linker error on libboost_python-vc120-mt-1_59.lib
when both python support and Matlab support are enable. It is OK to remove and I've pushed my changes.
@lunzueta please check my latest commit, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronzs, yes, I pulled your changes and merged with mine. Everything is fine in the last commit.
I think that boost-python needs to put back into matcaffe. I tried building matcaffe both Python and Matlab enabled in CommonSettings.props, and got a link error. |
I added the python_boost dependency to matcaffe and now it works also with PythonSupport activated (I don't understand why I couldn't before). I've updated README.md too. |
Can you try running the unit tests by invoking caffe.run_tests()? That should work in Matlab 2015a and 2015b, but for me it does not for some reason. EDIT: Actually all tests pass if test_solver.m is modified in line 16 by replacing |
@mnidza I just tried it in Matlab 2014b and 2015b, with the change you mention for test_solver.m, and it works fine in both. Otherwise, I got an error too. |
@aaronzs yes, that's right. With |
@@ -64,6 +72,11 @@ | |||
<PreprocessorDefinitions>WITH_PYTHON_LAYER;BOOST_PYTHON_STATIC_LIB;%(PreprocessorDefinitions)</PreprocessorDefinitions> | |||
</ClCompile> | |||
</ItemDefinitionGroup> | |||
<ItemDefinitionGroup Condition="'$(MatlabSupport)'=='true'"> | |||
<ClCompile> | |||
<PreprocessorDefinitions>MATLAB_MEX_FILE;%(PreprocessorDefinitions)</PreprocessorDefinitions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is MATLAB_MEX_FILE
necessary? I can't find it anywhere in source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not necessary, I added it because I'm not sure.
I found it when I was using MATLAB mex
to compile .cpp
files. I think the default MATLAB setting is:
CMDLINE100 : cl /c /Zp8 /GR /W3 /EHs /nologo /MD /O2 /Oy- /DNDEBUG /D_CRT_SECURE_NO_DEPRECATE /D_SCL_SECURE_NO_DEPRECATE /D_SECURE_SCL=0 /DMATLAB_MEX_FILE -Iinclude -I"C:\Program Files\MATLAB\R2015aSP1\extern\include" ...
Without these some parameters we can also get valid caffe_.mexw64
.
EDIT: MATLAB_MEX_FILE
is used in <MATLAB_ROOT>/extern/include/matrix.h
line: 1113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronzs OK, than let's keep it.
I've just tested the last version of my repo with the changes you added and now I see that the MEX file generated when compiled in Debug also works correctly :-) |
|
||
echo MatlabPostBuild.cmd : copy matlab generated scripts to output. | ||
|
||
copy /y "%SOLUTION_DIR%..\matlab\+caffe\*.m" "%OUTPUT_DIR%matcaffe\+caffe" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to copy all of this to build folder? I think that classification_demo
and hdf5creation
are not needed, as they are just examples. +caffe\imagenet
seems to be only used by classification demo, so I would drop that as well.
I'm not sure about copying unit tests. We don't do it in Python. Is there a way to execute unit tests easily if only +caffe\private
is copied to Release?
Also, I notice that +caffe\private\caffe_.cpp
is also copied; that should definitely be excluded :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnidza @lunzueta If we need a clean release. We can change the prebuild and postbuild scripts to:
set SOLUTION_DIR=%~1%
set OUTPUT_DIR=%~2%
echo MatlabPreBuild.cmd : Create output directories for matlab scripts.
if not exist "%OUTPUT_DIR%\matcaffe" mkdir "%OUTPUT_DIR%\matcaffe"
if not exist "%OUTPUT_DIR%\matcaffe\+caffe" mkdir "%OUTPUT_DIR%\matcaffe\+caffe"
if not exist "%OUTPUT_DIR%\matcaffe\+caffe\private" mkdir "%OUTPUT_DIR%\matcaffe\+caffe\private"
set SOLUTION_DIR=%~1%
set OUTPUT_DIR=%~2%
echo MatlabPostBuild.cmd : copy matlab generated scripts to output.
@echo run_tests.m > "%temp%\excludelist.txt"
xcopy /y "%SOLUTION_DIR%..\matlab\+caffe\*.m" "%OUTPUT_DIR%matcaffe\+caffe" /exclude:%temp%\excludelist.txt
copy /y "%SOLUTION_DIR%..\matlab\+caffe\private\*.m" "%OUTPUT_DIR%matcaffe\+caffe\private"
move /y "%OUTPUT_DIR%caffe_.*" "%OUTPUT_DIR%matcaffe\+caffe\private"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnidza OK, I removed the copy of those files, but I'm not sure either about what to do with the test files, because although it's quite simple not to copy the +caffe\+test
folder, it's not so simple to exclude the run_tests.m
file from the rest of m
files of the same folder. According to this (http://stackoverflow.com/questions/4098444/move-all-files-except-some-file-pattern-from-a-dos-command) xcopy
could be used instead of copy
, but then a text file should be added which would include only that file to be excluded, which looks a bit strange to me. Wouldn't be good to copy those test files as well? I think they are quite useful to test in Matlab in an easy way if the compilation was good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronzs I just saw now your last response. OK, I'll update the scripts as you say.
I have a doubt about how to configure the Matlab search path to be able to use this, which probably will happen to more people (as can be seen here: rbgirshick/rcnn#56), if we don't explain it clearly in README.md. When I run |
@lunzueta On my computer there is no problem just add |
@aaronzs Yes, you are right! I just commented lines 58-62 and it works now. So, then I'd say that the explanation given in README.md is good enough. |
@@ -45,7 +45,11 @@ void mxCHECK_FILE_EXIST(const char* file) { | |||
static vector<shared_ptr<Solver<float> > > solvers_; | |||
static vector<shared_ptr<Net<float> > > nets_; | |||
// init_key is generated at the beginning and everytime you call reset | |||
static double init_key = static_cast<double>(caffe_rng_rand()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to understand exactly what causes MATLAB hang, and how this modification helps prevent that. Then we could have a more detailed/informative comment. I tried the unmodified version (CPU-only build) on MATLAB 2015a and managed to run unit tests without any issues. Maybe problems arise only in GPU build.
In any case, the sentences in the comment should be properly capitalized, and 'native' should be changed to 'negative'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnidza Sorry about the typo...
It seems that MATLAB hang on line 112 in common.cpp
when cublasCreate(&cublas_handle_)
is called. Set CPU_ONLY
will get rid of this part, so CPU-only build have no issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use _MSC_VER
to ensure that the modification is enabled only on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnidza Sure. Shall we use _MSC_VER
or _WIN32
?
Edit: I find _MSC_VER
is used in rest part of codes. I will use _MSC_VER
@aaronzs with the last update I'm getting the following compilation error in VS2013, which I didn't get in a previous version: |
@lunzueta Did you use the latest |
@aaronzs OK, I didn't update that file with the newer version. It works fine now. Thanks. |
Ok seems like all issue have been resolved, let's squash all the commits into a single one. |
Necessary changes to enable matcaffe build in Windows: - Visual Studio project file added. - Common settings file updated. - Pre and post build scripts added. - 3rd party dependencies resolved through NuGet. - Minor code changes.
matcaffe VS project and scripts added.
This pull request is related to issue #19.
I created a VS project and the corresponding scripts, in a similar way to those for pycaffe, to be able to compile matcaffe with VS2013. For that I also took into account a couple of things from (https://github.com/happynear/caffe-windows) to export the mex file correctly. With this compilation I could run the classification_demo.m successfully in Matlab 2014b.