Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] New MATLAB wrapper #407

Closed
7 tasks done
ProfFan opened this issue Jul 14, 2020 · 20 comments
Closed
7 tasks done

[WIP] New MATLAB wrapper #407

ProfFan opened this issue Jul 14, 2020 · 20 comments
Assignees
Labels
bug Bug report help wanted Need help and/or clarification matlab Related to MATLAB wrapper
Milestone

Comments

@ProfFan
Copy link
Collaborator

ProfFan commented Jul 14, 2020

We need to finish this before 4.1.

  • code generation for some method signatures is broken (wrong type)
  • matlab compiles (only for gtsam, gtsam_unstable is broken even in develop)
  • we have not tested on MATLAB at all (I don't know how to load the module)
  • resolve overloads or add numbering?
  • resolve the issue where the numbering of C++ and matlab functions mismatch
  • resolve the crashes on factor construction
  • This declarations are broken because the concrete type is dropped during parsing
@ProfFan ProfFan self-assigned this Jul 14, 2020
@ProfFan ProfFan added the bug Bug report label Jul 14, 2020
@ProfFan ProfFan added this to the GTSAM 4.1 milestone Jul 14, 2020
@ProfFan ProfFan linked a pull request Jul 14, 2020 that will close this issue
3 tasks
@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 14, 2020

@dellaert

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 14, 2020

I have updated the feature/new_wrapper branch to reflect the changes.

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 14, 2020

I have finished the wrapping for matlab and briefly tested on MATLAB (loading and simple function works), could anyone help me test it by:

  • Clone the feature/new_wrapper branch
  • git submodule update --init --recursive
  • mkdir build && cd build
  • cmake -DCMAKE_BUILD_TYPE=Release -DGTSAM_TYPEDEF_POINTS_TO_VECTORS=ON -DGTSAM_INSTALL_MATLAB_TOOLBOX=ON -DCMAKE_INSTALL_PREFIX=../gtsam_install -DGTSAM_CUSTOM_MATLAB_PATH=<your matlab>/MATLAB/R2020a/bin ..
  • make -j8 install
  • Go to ../gtsam_install/gtsam_toolbox and run the matlab wrapper

And report the issues you have? Many thanks!

@varunagrawal @mikesheffler @dellaert

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 14, 2020

I am getting errors like this when running the examples:

PlanarSLAMExample
Error using gtsam.NonlinearFactorGraph
Error: File: NonlinearFactorGraph.m Line: 341 Column: 5
'push_back' is already defined as a method in the 'NonlinearFactorGraph' class.

Error in PlanarSLAMExample (line 29)
graph = NonlinearFactorGraph;

So we probably need to do some name mangling here.

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 14, 2020

Same goes for the file name resolution. Really need some help from someone with better MATLAB experience...

@ProfFan ProfFan added the help wanted Need help and/or clarification label Jul 14, 2020
@varunagrawal
Copy link
Collaborator

I'll take a look at this later today.

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 14, 2020

@varunagrawal Thank you in advance :) I need a list of problems so I can tweak the generation code...

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 16, 2020

Another rather serious issue is that the current generator will generate identical signature for overloaded function, which is illegal in MATLAB:

    function varargout = RzRyRx(varargin)
      % RZRYRX usage: RzRyRx(double x, double y, double z) : returns gtsam::Rot3
      % Doxygen can be found at http://research.cc.gatech.edu/borg/sites/edu.borg/html/index.html
      varargout{1} = gtsam_wrapper(263, varargin{:});
    end

    function varargout = RzRyRx(varargin)
      % RZRYRX usage: RzRyRx(Vector xyz) : returns gtsam::Rot3
      % Doxygen can be found at http://research.cc.gatech.edu/borg/sites/edu.borg/html/index.html
      varargout{1} = gtsam_wrapper(264, varargin{:});
    end

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 16, 2020

Now most of the API is working, but construction of a simple factor crashes MATLAB:

import gtsam.*

graph = NonlinearFactorGraph;

%% Add prior
priorNoise = noiseModel.Diagonal.Sigmas([0.3; 0.3; 0.1]);
graph.add(PriorFactorPose2(1, Pose2(0, 0, 0), priorNoise)); % Crashes here

I'll keep investigating what is happening, but any help is welcome...

@dellaert
Copy link
Member

I'm puzzled because many of these issues were not present for the c++ based wrap, and I thought we generated nearly identical c++ wrapper files. So, what's up with that? Let's talk in our meeting.

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 16, 2020

@dellaert Sure, it's a lot of delicate name mangling differences....

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 16, 2020

Now the crash is fixed and the examples mostly work.

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 22, 2020

Since 4.0.3 is out, we now need testers for the new MATLAB wrapper, since the generated code currently does not exactly match what is generated by the old C++ wrapper.

@dellaert
Copy link
Member

Since 4.0.3 is out, we now need testers for the new MATLAB wrapper, since the generated code currently does not exactly match what is generated by the old C++ wrapper.

It would be nice if this call came with instructions and a concrete ask :-) When fixed, also announce on internal slack?

@ProfFan
Copy link
Collaborator Author

ProfFan commented Jul 22, 2020

@dellaert Sure.

@varunagrawal varunagrawal added the matlab Related to MATLAB wrapper label Jan 2, 2021
@varunagrawal varunagrawal linked a pull request Mar 10, 2021 that will close this issue
@varunagrawal
Copy link
Collaborator

@ProfFan I checked the issue with This declarations and it seems to work fine. I believe we can close this issue. 🙂

@varunagrawal
Copy link
Collaborator

Nvm I encountered it again. I can fix it after I get the CI up and running again.

@varunagrawal
Copy link
Collaborator

The This issue only happens when the class isn't templated, so that helps a lot.

@varunagrawal
Copy link
Collaborator

Also, FYI I fixed all the Matlab tests so the tests now run and they pass.

@varunagrawal
Copy link
Collaborator

This issue has been resolved 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report help wanted Need help and/or clarification matlab Related to MATLAB wrapper
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants