Fix Matlab Wrapper

[varunagrawal]

gtwrap updates and other fixes to get the GTSAM matlab wrapper working.

@mattking-smith can you please pull this branch to your system and check? Everything should work at this point.
If you can confirm that everything works, you can approve this PR and we can merge it.

[ProfFan]

@varunagrawal Did you run MATLAB unit tests and examples? If so we can merge, need to cut the release very soon.

[varunagrawal]

I ran the unit tests but not the examples.

[ProfFan]

This looks good to me!

[dellaert]

@mattking-smith this PR seems ready to go, but is just awaiting your confirmation it does work :-)

[johnwlambert]

Thanks for the fixes Varun. I wonder if we can use octave in the CI to catch these sort of things in the future?

[mattking-smith]

Although I was able to successfully run the test cases under /home/username/toolbox/gtsam_tests and generate the following output:

>> test_gtsam
Starting: testCal3Unified
Starting: testKalmanFilter
Starting: testJacobianFactor
Starting: testValues
Starting: testPriorFactor
Starting: testLocalizationExample
Starting: testOdometryExample
Starting: testPlanarSLAMExample
Starting: testPose2SLAMExample
Starting: testPose3SLAMExample
Starting: testSFMExample
Starting: testStereoVOExample
Starting: testVisualISAMExample
Starting: testUtilities
Starting: testSerialization
Tests complete!

I am having some errors running some of the GTSAM examples (/home/username/toolbox/gtsam_examples). Specifically, I am seeing the following error:

> CameraFlyingExample

Error using gtsam.noiseModel.Diagonal.Sigmas (line 109)
Arguments do not match any overload of function
Diagonal.Sigmas

Error in gtsam.points2DTrackStereo (line 15)
posePriorNoise  =
noiseModel.Diagonal.Sigmas(poseNoiseSigmas);

Error in CameraFlyingExample (line 170)
    pts2dTracksStereo =
    points2DTrackStereo(options.camera.stereoK, ...

This error persists for all examples containing the noiseModel class.

Was the noiseModel class changed and not updated on the MATLAB side?

[dellaert]

@mattking-smith Thanks! Yes, that could be the case. Since now the tests work, I will merge this PR, But could I ask you to create an issue to fix the Matlab examples.?

[dellaert]

Thanks for the fixes Varun. I wonder if we can use octave in the CI to catch these sort of things in the future?

I think that’s a great idea, but we would need some volunteer to implement it :-) I do not want Varun to do this before RSS. Ideally it’s somebody who is invested as they are using the Matlab toolbox on a daily basis.

[mattking-smith]

This appears to fix the wrapper and linking of GTSAM + MATLAB.

However, I am seeing that errors when testing all the GTSAM examples in the gtsam_examples folder.

For example when looking at StereoVOExample.m when trying to execute
stereo_model = noiseModel.Isotropic.Sigma(3,1);
as written when compiled throws the error:

>> stereo_model = noiseModel.Isotropic.Sigma(3,1);
Error using gtsam.noiseModel.Isotropic.Sigma (line 76)
Arguments do not match any overload of function
Isotropic.Sigma

However looking at the wrapper definition under Isotropic.m states
%Sigma(size_t dim, double sigma, bool smart) : returns gtsam::noiseModel::Isotropic
where the Isotropic wrapper definition in MATLAB is:

    function varargout = Sigma(varargin)
      % SIGMA usage: Sigma(size_t dim, double sigma, bool smart) : returns gtsam.noiseModel.Isotropic
      % Doxygen can be found at https://gtsam.org/doxygen/
      if length(varargin) == 3 && isa(varargin{1},'numeric') && isa(varargin{2},'double') && isa(varargin{3},'logical')
        varargout{1} = gtsam_wrapper(1092, varargin{:});
        return
      end

      error('Arguments do not match any overload of function Isotropic.Sigma');
    end

Hence executing
>> stereo_model = noiseModel.Isotropic.Sigma(3,1,true);
is handled without error and GTSAM can properly optimize the StereoVOExample.m file:

Optimizing
Elapsed time is 0.014737 seconds.

So yes the wrap fixes the linking, however it appear that some wrapper definitions have changed and the baseline example codes in MATLAB need to updated to reflect this.

[mattking-smith]

@mattking-smith Thanks! Yes, that could be the case. Since now the tests work, I will merge this PR, But could I ask you to create an issue to fix the Matlab examples.?

I am just seeing this. I can move this issue to the appropriate form

[varunagrawal]

@johnwlambert yes I have actually considered that from a year ago(!!) but Octave has a slightly different wrapping process and the time investment vs reward gained was not sufficient for me. My job is research after all and we use the python wrapper more. 🙂
If you have a necessary use for the Matlab wrapper, then we could consider a plan for using octave in the CI, but a better solution would be to use local Github runners where we link a local computer with Matlab on it to Github Actions so that Actions can run the CI on that computer.

[ProfFan]

I already tried octave for CI 2 years ago, it's not worth it... Maybe octave has evolved, but someone (not me or varun) should investigate.

[varunagrawal]

@mattking-smith I forgot to mention this due to the time crunch: the matlab wrapper does not yet support default arguments. This is why you're seeing the issue. The .i files allow for specifying default arguments to various parameters and gtwrap can take those into account. This happens for the python wrapper but not yet for the matlab wrapper.

[varunagrawal]

@ProfFan @dellaert yeah if someone wants something, they should probably own it and make it happen. We're all equally qualified here. 🙂

[dellaert]

@mattking-smith I forgot to mention this due to the time crunch: the matlab wrapper does not yet support default arguments. This is why you're seeing the issue. The .i files allow for specifying default arguments to various parameters and gtwrap can take those into account. This happens for the python wrapper but not yet for the matlab wrapper.

But why would that create a failure? The examples were written before default arguments existed.

[mattking-smith]

rgot to mention this due to the time crunch: the matlab wrapper does not yet support default arguments. This is why you're seeing the issue. The .i files allow for specifying default arguments to various parameters and gtwrap can take those into account. This happens for the python wrapper but not yet for the matlab wrapper.

So am I still trying to open an issue in the matlab issue section?

will merge this PR, But could I ask you to create an issue to fix the Matlab examples.?

@mattking-smith I forgot to mention this due to the time crunch: the matlab wrapper does not yet support default arguments. This is why you're seeing the issue. The .i files allow for specifying default arguments to various parameters and gtwrap can take those into account. This happens for the python wrapper but not yet for the matlab wrapper.

But why would that create a failure? The examples were written before default arguments existed.

Should I be pointing out this MATLAB bug in a new issue?

[dellaert]

I think it would be good if you guys talked for five minutes over blue jeans. If default arguments in the wrapper caused the matlab wrapper to fail, then that is an urgent bug but that needs to be fixed, so yes please create an issue John is correct to point out that the lack of CI makes this possible without us knowing it. From a process point of view, the people that change the wrapper, should then count on compiling the wrapper on their own machine at least. No blame here, just what we should do moving forward.

[varunagrawal]

@mattking-smith for now just add the missing arguments and it should work.

@dellaert default arguments to Python were added earlier in the spring, and not to Matlab (this is my bad). The fix is simple though to just add the argument into the scripts. I can figure out matlab default arguments later this week after I finish the higher priority items.

[dellaert]

I think it would be good if you guys talked for five minutes over blue jeans: maybe Matt could fix this in wrap and get some insight into how the wrapper is generated...