Matlab default arguments #135

gchenfc · 2021-12-09T05:32:49Z

This addresses #134

It is done by creating multiple copies of the same function then letting the normal overload handling functionality handle the switch cases for argument deduction.

For a function with a default argument, it's expanded to become the equivalent of:

void function(int a, int b = 0);
// turns into:
void function(int a, int b);
void function(int a);

Then in the generated .cpp file, for the second version we actually call function(a, 0).

2 things to look out for:

"creating copies" requires care due to the parent-child cyclic references (cannot naively use deepcopy)
for the function version where the argument is omitted, we want to "delete" the optional argument. But this comes with the minor caveat that, when generating the cpp file, we need to explicitly pass the argument, e.g. function(a, 0) means we need to "remember" the 0. To do this, I just create a copy of the virgin argument list and we can get the default values from there.

Haven't tested it yet with gtsam since I haven't configured the matlab wrapper on my computer yet (need to recompile everything including dependencies with intel) but I think it should work.

tag @mattking-smith

gchenfc · 2021-12-09T06:00:21Z

Would also close borglab/gtsam#954 which in turn is holding up 4.1 release (borglab/gtsam#824).

mattking-smith · 2021-12-09T12:49:59Z

Haven't tested it yet with gtsam since I haven't configured the matlab wrapper on my computer yet (need to recompile everything including dependencies with intel) but I think it should work.

I am going to test this now.

varunagrawal · 2021-12-09T16:03:35Z

Nice work! I was going to work on this after I got some immediate stuff out of the way, so thanks.

Can you explain the issue about copying and child-parent cyclical dependencies?

varunagrawal

Needs more comments for sure, but we should also talk about the order of handling of default arguments. I think if we consider that optional args always go after default args, this is PR can be vastly simplified.

gtwrap/matlab_wrapper/wrapper.py

varunagrawal · 2021-12-09T16:11:05Z

gtwrap/matlab_wrapper/wrapper.py

+                methodWithArg = method_copy(method)
+                method.args.list().remove(arg)
+                return [
+                    *MatlabWrapper._expand_default_arguments(methodWithArg, save_backup=False),


I am pretty sure you can call this instead of MatlabWrapper. Let me double check though.

this is in a static function so it doesn't have a self variable, unless there's a keyword this that I didn't know about in python?

Oops didn't see that. Fair enough, but then what is the reason for this being a staticmethod in the first place?

it doesn't need member variables?

I guess when I write python code I tend to make everything static unless it needs member functions/variables, but I guess it's a style thing? Just felt more functional and "proper" to me.

I can change it if you want but also I don't really see a problem with it

You're still calling a class method. If the class method needs an instance variable then the calling method shouldn't be static. If this method and it's constituents don't depend on any instance variable, then you could make this a classmethod instead. Same deal, but you can then use cls to access the method internally which makes semantic sense.

I don't follow what you mean...

no I'm calling a static method, which is different than a class method in python

the method does not need an instance variable, if it did then this wouldn't work?

sure I could make this a class method but why? I think this is a pretty standard usage of staticmethod and this function doesn't need access to any class state or anything (e.g. class static variables) so I think staticmethod is more appropriate here? basically this function could be outside of the class entirely and it would behave exactly the same, I just included it inside the class because it semantically makes sense to be there. I feel like that's pretty much the definition of a static function

I kind of feel like this is a pretty minor detail to get hung up on?

It is not major for sure, but I wanted to double check. There's some misunderstanding here: I wanted to clarify that expand_default_arguments wasn't using an instance variable in some way.

Generally static methods are used when you just want encapsulation of some functionality. Since you are calling a class method inside this method, it seems more appropriate to do it as a classmethod so you can do cls.expand_default_arguments.

But yeah this is minor and it doesn't make any difference whether it is updated or not.

Just to clarify, which class method is getting called from inside this method? _expand_default_arguments is a static method so it's still calling a static method?

feel free to resolve if you feel ok with it

varunagrawal · 2021-12-09T16:12:31Z

tests/expected/matlab/DefaultFuncInt.m

@@ -1,6 +1,12 @@
 function varargout = DefaultFuncInt(varargin)
      if length(varargin) == 2 && isa(varargin{1},'numeric') && isa(varargin{2},'numeric')
        functions_wrapper(8, varargin{:});
+      elseif length(varargin) == 1 && isa(varargin{1},'numeric')
+        functions_wrapper(9, varargin{:});
+      elseif length(varargin) == 1 && isa(varargin{1},'numeric')


Isn't it a C++ syntax thing where optional arguments always need to go after required arguments? Doesn't matlab do the same thing?

matlab is weird since it's left to the function implementation to decide how to parse arguments, so you can pretty much do whatever you want w.r.t. optional arguments. Though it is true that there will be many of these if-blocks that are impossible to reach, but I figured the added flexibility with optional arguments was worth the useless if-blocks.

I think I left a comment in the recursion step of expand_default_arguments saying "disable this expand to disallow skipping arguments" or something like that which would change the behavior to what you propose.

But then how would you disambiguate those two if conditions? If I call DefaultFuncInt(b), it would still hold true for the first elseif block and never go to the next elseif block. Or am I missing something here?

I think just always taking precedence for the first argument being specified and the second skipped (vs first skipped and specifying second) is a reasonable policy which is implemented currently.

i.e. the DefaultFuncInt(default, b) will never get called (aka impossible to reach).

A truly matlab-ic way to do things would be allow empty lists to be placeholders (instead of boost::none), e.g.
DefaultFuncInt([], b). I think this is probably best put into a different PR, but I guess I could implement that in this PR if you want? (in additional to some other normal overloading rules).

I mean basically the only difference between what you're proposing and what I've implemented is that, for example,

func(int a = 0, string s = "")

can be called as func("non-default string") in this implementation whereas it cannot be called that way in your more strict parsing proposal. Everything else is the same.

The only caveat is if there happens to be a different, custom function overload func(string s) in which case overload resolution will depend on the order the functions were defined in the .i file, but then that seems a little bit dubious anyway.

I think just always taking precedence for the first argument being specified and the second skipped (vs first skipped and specifying second) is a reasonable policy which is implemented currently.

i.e. the DefaultFuncInt(default, b) will never get called (aka impossible to reach).

I am not sure how this is a reasonable policy. If I'm a matlab user, you're telling me for DefaultFuncInt(int a, int b) that DefaultFuncInt(a) and DefaultFuncInt(b) are both the same underlying call, when my intention is clearly different. Moreover this is would add bloat to the wrapper and make it confusing for the next person who comes along to update this. If a particular code path is never accessed, it shouldn't exist.

I mean basically the only difference between what you're proposing and what I've implemented is that, for example,

func(int a = 0, string s = "")

can be called as func("non-default string") in this implementation whereas it cannot be called that way in your more strict parsing proposal. Everything else is the same.

The only caveat is if there happens to be a different, custom function overload func(string s) in which case overload resolution will depend on the order the functions were defined in the .i file, but then that seems a little bit dubious anyway.

This caveat is important because users of the wrapper wouldn't know about the underlying semantics of which overload is being called. If we specify overloaded functions in the .i file in a different order, we shouldn't see different behavior, right? That's just going to make using the wrapper painful.

By forcing users to call func(0, "non-default string") from their Matlab script also makes their code more readable, makes our job of maintaining the wrapper easier, and removes all doubt about ambiguities, which is why I am suggesting implementing the C++-like strategy rather than the matlab-like strategy. It's a really cool feature, but it seems prone to lots of errors and the matlab wrapper is a pain to maintain anyway.

ok let me change it to the c++ way

Just fyi @varunagrawal I just realized there's a test that's illegal in c++ so I changed that.

varunagrawal · 2021-12-09T16:13:07Z

tests/expected/matlab/functions_wrapper.cpp

+{
+  checkArguments("DefaultFuncInt",nargout,nargin,1);
+  int b = unwrap< int >(in[0]);
+  DefaultFuncInt(123,b);


Based on my previous comment, I don't think this scenario will ever happen, unless you have python-like keyword arguments.

Ya - see above comment that I think the added flexibility is worth the tradeoff that we will get some blocks that will never get called.

gchenfc · 2021-12-09T16:14:54Z

ya,

so if there's a parsed method declaration of the form

func(int a, int b = 0)

then I am duplicating that into 2 objects:

func(int a, int b)
func(int a)

This "duplication step" requires copying the original func object. But copying is difficult because:

a method has a member variable of type ArgumentList, but ArgumentList has a member variable parent that points back to the method. So if you try to use copy.deepcopy, then you'll get some infinite recursion or something as method tries to copy args, but then while making a copy of args, it'll try to make another copy of method, but to make a copy of method, it'll needs to make a copy of args, etc. We get the same issue with ArgumentList and Argument, and pretty much every parsed type in this library.

So then I defined some custom copy functions that only copy the data that I'm going to modify and leaves the rest of the variables as references. Although the "right" way to do this would probably be to implement a copy() method in each class, I felt like this would get hairy since it's not always clear what variables you want to keep as references vs true copies and in this case we don't have to make true copies of everything, we only need to make true copies of a few data.

mattking-smith

I am having trouble compiling this current version of the wrap module. To double check I have correctly checked out this repository before fully coming to this conclusion, these are my steps:

Use command prompt and check gtsam branch:

matt@matt-Satellite-S55-C:/usr/local/gtsam$ git branch
* develop
  feature/matlab_wrapper
  feature/wrap/extractvectors
  fix/MATLAB_default_arguments
  fix/matlab-wrapper
  master

Change to wrap directory and get git branch.

matt@matt-Satellite-S55-C:/usr/local/gtsam/wrap$ git branch
* feature/matlab_default_args
  master

Change to build folder and try and make and install: matt@matt-Satellite-S55-C:/usr/local/gtsam/build$ sudo make install -j 4

I am getting the following error before the end of compilation though:

[ 94%] Built target gtsam_unstable_py
[ 97%] Built target SmartRangeExample_plaza1
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:518:43: error: ‘RedirectCout’ is not a member of ‘gtsam’
  518 | typedef std::set<boost::shared_ptr<gtsam::RedirectCout>*> Collector_gtsamRedirectCout;
      |                                           ^~~~~~~~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:518:43: error: ‘RedirectCout’ is not a member of ‘gtsam’
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:518:55: error: template argument 1 is invalid
  518 | typedef std::set<boost::shared_ptr<gtsam::RedirectCout>*> Collector_gtsamRedirectCout;
      |                                                       ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:518:57: error: template argument 1 is invalid
  518 | typedef std::set<boost::shared_ptr<gtsam::RedirectCout>*> Collector_gtsamRedirectCout;
      |                                                         ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:518:57: error: template argument 2 is invalid
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:518:57: error: template argument 3 is invalid
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void _deleteAllObjects()’:
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1198:47: error: qualified-id in declaration before ‘iter’
 1198 |   { for(Collector_gtsamRedirectCout::iterator iter = collector_gtsamRedirectCout.begin();
      |                                               ^~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1198:46: error: expected ‘;’ before ‘iter’
 1198 |   { for(Collector_gtsamRedirectCout::iterator iter = collector_gtsamRedirectCout.begin();
      |                                              ^~~~~
      |                                              ;
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1198:47: error: ‘iter’ was not declared in this scope
 1198 |   { for(Collector_gtsamRedirectCout::iterator iter = collector_gtsamRedirectCout.begin();
      |                                               ^~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1198:82: error: request for member ‘begin’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 1198 | llector_gtsamRedirectCout::iterator iter = collector_gtsamRedirectCout.begin();
      |                                                                        ^~~~~

/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1199:43: error: request for member ‘end’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 1199 |       iter != collector_gtsamRedirectCout.end(); ) {
      |                                           ^~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1199:48: error: expected ‘)’ before ‘;’ token
 1199 |       iter != collector_gtsamRedirectCout.end(); ) {
      |                                                ^
      |                                                )
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1198:8: note: to match this ‘(’
 1198 |   { for(Collector_gtsamRedirectCout::iterator iter = collector_gtsamRedirectCout.begin();
      |        ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1199:50: error: expected primary-expression before ‘)’ token
 1199 |       iter != collector_gtsamRedirectCout.end(); ) {
      |                                                  ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void gtsamRedirectCout_collectorInsertAndMakeBase_103(int, mxArray**, int, const mxArray**)’:
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4240:36: error: ‘RedirectCout’ is not a member of ‘gtsam’
 4240 |   typedef boost::shared_ptr<gtsam::RedirectCout> Shared;
      |                                    ^~~~~~~~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4240:36: error: ‘RedirectCout’ is not a member of ‘gtsam’
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4240:48: error: template argument 1 is invalid
 4240 |   typedef boost::shared_ptr<gtsam::RedirectCout> Shared;
      |                                                ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4243:31: error: request for member ‘insert’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 4243 |   collector_gtsamRedirectCout.insert(self);
      |                               ^~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void gtsamRedirectCout_constructor_104(int, mxArray**, int, const mxArray**)’:
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4249:36: error: ‘RedirectCout’ is not a member of ‘gtsam’
 4249 |   typedef boost::shared_ptr<gtsam::RedirectCout> Shared;
      |                                    ^~~~~~~~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4249:36: error: ‘RedirectCout’ is not a member of ‘gtsam’
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4249:48: error: template argument 1 is invalid
 4249 |   typedef boost::shared_ptr<gtsam::RedirectCout> Shared;
      |                                                ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4251:33: error: expected type-specifier
 4251 |   Shared *self = new Shared(new gtsam::RedirectCout());
      |                                 ^~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4252:31: error: request for member ‘insert’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 4252 |   collector_gtsamRedirectCout.insert(self);
      |                               ^~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void gtsamRedirectCout_deconstructor_105(int, mxArray**, int, const mxArray**)’:
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4259:36: error: ‘RedirectCout’ is not a member of ‘gtsam’
 4259 |   typedef boost::shared_ptr<gtsam::RedirectCout> Shared;
      |                                    ^~~~~~~~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4259:36: error: ‘RedirectCout’ is not a member of ‘gtsam’
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4259:48: error: template argument 1 is invalid
 4259 |   typedef boost::shared_ptr<gtsam::RedirectCout> Shared;
      |                                                ^
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4262:41: error: qualified-id in declaration before ‘item’
 4262 |   Collector_gtsamRedirectCout::iterator item;
      |                                         ^~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4263:3: error: ‘item’ was not declared in this scope; did you mean ‘boost::ptr_container_detail::item’?
 4263 |   item = collector_gtsamRedirectCout.find(self);
      |   ^~~~
      |   boost::ptr_container_detail::item
In file included from /usr/include/boost/ptr_container/detail/serialize_ptr_map_adapter.hpp:10,
                 from /usr/include/boost/ptr_container/serialize_ptr_map.hpp:9,
                 from /usr/local/gtsam/gtsam/nonlinear/Values.h:33,
                 from /usr/local/gtsam/gtsam/nonlinear/NonlinearFactor.h:23,
                 from /usr/local/gtsam/gtsam/nonlinear/FunctorizedFactor.h:21,
                 from /usr/local/gtsam/gtsam/basis/BasisFactors.h:22,
                 from /usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:13:
/usr/include/boost/ptr_container/detail/serialize_xml_names.hpp:20:28: note: ‘boost::ptr_container_detail::item’ declared here
   20 |         inline const char* item()   { return "item"; }
      |                            ^~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4263:38: error: request for member ‘find’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 4263 |   item = collector_gtsamRedirectCout.find(self);
      |                                      ^~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4264:42: error: request for member ‘end’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 4264 |   if(item != collector_gtsamRedirectCout.end()) {
      |                                          ^~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4266:33: error: request for member ‘erase’ in ‘collector_gtsamRedirectCout’, which is of non-class type ‘Collector_gtsamRedirectCout’ {aka ‘int’}
 4266 |     collector_gtsamRedirectCout.erase(item);
      |                                 ^~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void gtsamRedirectCout_str_106(int, mxArray**, int, const mxArray**)’:
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4273:39: error: ‘RedirectCout’ is not a member of ‘gtsam’
 4273 |   auto obj = unwrap_shared_ptr<gtsam::RedirectCout>(in[0], "ptr_gtsamRedirectCout");
      |                                       ^~~~~~~~~~~~
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4273:39: error: ‘RedirectCout’ is not a member of ‘gtsam’
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4273:83: error: no matching function for call to ‘unwrap_shared_ptr<<expression error> >(const mxArray*&, const char [22])’
 4273 | hared_ptr<gtsam::RedirectCout>(in[0], "ptr_gtsamRedirectCout");
      |                                                              ^

In file included from /usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:1:
/usr/local/gtsam/build/wrap/matlab.h:468:26: note: candidate: ‘template<class Class> boost::shared_ptr<X> unwrap_shared_ptr(const mxArray*, const string&)’
  468 | boost::shared_ptr<Class> unwrap_shared_ptr(const mxArray* obj, const string& propertyName) {
      |                          ^~~~~~~~~~~~~~~~~
/usr/local/gtsam/build/wrap/matlab.h:468:26: note:   template argument deduction/substitution failed:
/usr/local/gtsam/build/wrap/gtsam/gtsam_wrapper.cpp:4273:83: error: template argument 1 is invalid
 4273 | hared_ptr<gtsam::RedirectCout>(in[0], "ptr_gtsamRedirectCout");
      |                                                              ^

make[2]: *** [matlab/CMakeFiles/gtsam_matlab_wrapper.dir/build.make:79: matlab/CMakeFiles/gtsam_matlab_wrapper.dir/__/wrap/gtsam/gtsam_wrapper.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:31966: matlab/CMakeFiles/gtsam_matlab_wrapper.dir/all] Error 2
make: *** [Makefile:163: all] Error 2

Am I just working with the wrong wrap submodule or something?

gchenfc · 2021-12-09T17:14:05Z

Hmm not sure, this seems like a longer thing to figure out - this evening I'll get the matlab wrapper up and running on one of my machines so I can debug more directly. I'll keep you posted.

varunagrawal · 2021-12-09T17:27:05Z

@mattking-smith if you're compiling GTSAM, it would be using the in-built wrapper under the wrap directory. The changes here would need to either manually copied or pulled into the wrap directory using a git subtree command.

@gchenfc for the deepcopy thing, we should see if we can just remove that parent pointer. I don't think that's possible because of the templating need, but still worth taking a look. This is an opportunity to make the code cleaner in more ways than one! 😄

gchenfc · 2021-12-09T17:34:51Z

@gchenfc for the deepcopy thing, we should see if we can just remove that parent pointer. I don't think that's possible because of the templating need, but still worth taking a look. This is an opportunity to make the code cleaner in more ways than one! 😄

Sounds good, but could we move this to an issue and leave the current copy-ing mechanism as-is in this PR? I would prefer to keep this PR focused on only one change at a time. Or if you have comments on the copying as it is in this PR independent of larger, tangential refactors.

Since this is also potentially holding up GTSAM 4.1 release, I guess it would also be better to handle large refactors later?

mattking-smith · 2021-12-09T18:40:32Z

@mattking-smith if you're compiling GTSAM, it would be using the in-built wrapper under the wrap directory. The changes here would need to either manually copied or pulled into the wrap directory using a git subtree command.

@varunagrawal is this the correct subtree approach?:

matt@matt-Satellite-S55-C:/usr/local/gtsam$ git subtree pull --prefix wrap https://github.com/borglab/wrap.git feature/matlab_default_args --squash

varunagrawal · 2021-12-09T18:43:04Z

I'm not sure and I don't recommend it since you'll have to undo the pull anyway. A simple copy of the python files should suffice.

mattking-smith · 2021-12-09T18:46:12Z

I'm not sure and I don't recommend it since you'll have to undo the pull anyway. A simple copy of the python files should suffice.

Right now I am getting this when I try the pull again:

matt@matt-Satellite-S55-C:/usr/local/gtsam$ git subtree pull --prefix wrap https://github.com/borglab/wrap.git feature/matlab_default_args --squash
From https://github.com/borglab/wrap
 * branch                feature/matlab_default_args -> FETCH_HEAD
Subtree is already at commit 6166dfa4d9a4c92f1663a403609a7a5829adb6ab.

I realize that this means I have to undo this pull. But it copied the updated python files. I will try and recompile and see if I messed up my original compilation this morning.

mattking-smith · 2021-12-09T18:52:51Z

Hmm not sure, this seems like a longer thing to figure out - this evening I'll get the matlab wrapper up and running on one of my machines so I can debug more directly. I'll keep you posted.

@gchenfc when you setup your MATLAB wrapper again, can you go see if you can reproduce the error given in #133 (comment)? Just to make sure the toolbox memory leak isn't only on my side of things.

mattking-smith

@gchenfc After going back through and pulling the subtree of the wrap fix into my repository via:
git subtree pull --prefix wrap https://github.com/borglab/wrap.git feature/matlab_default_args --squash
Then I was able to successfully build + test your code. Everything is working fine. I would say this is a successful fix to this and the open issue #134 (comment)

mattking-smith · 2021-12-09T20:01:49Z

If @varunagrawal is satisfied, I would say this is good for merging @gchenfc.

varunagrawal · 2021-12-09T20:18:49Z

Let's hold on on the merging. We want to figure some things out.

mattking-smith · 2021-12-09T22:17:26Z

Let's hold on on the merging. We want to figure some things out.

Would it be possible to have these revisions done soon? I am trying to run some experiments in my lab with some the new functionality of GTSAM (extract vectors), but I do not want to merge my development computer with the development branch until this is resolved...

varunagrawal · 2021-12-10T02:26:58Z

@mattking-smith I feel you but we need to do these things right because there will be repercussions beyond us if we don't.

mattking-smith · 2021-12-10T02:34:38Z

@mattking-smith I feel you but we need to do these things right because there will be repercussions beyond us if we don't.

That makes a lot of sense. Thanks for all the help so far. Thankfully I can run my code on another computer that is a little behind this current wrap version.

…ding to c++ rules)

mattking-smith · 2021-12-12T04:15:58Z

@varunagrawal Have you had a chance to review @gchenfc changes to this code?

varunagrawal

Amazing stuff @gchenfc. Thanks for taking my feedback into account, I don't know about you but I think the new generated code looks incredible!

varunagrawal · 2021-12-13T13:51:51Z

tests/expected/matlab/setPose.m

@@ -1,6 +1,8 @@
 function varargout = setPose(varargin)
      if length(varargin) == 1 && isa(varargin{1},'gtsam.Pose3')
-        functions_wrapper(13, varargin{:});
+        functions_wrapper(23, varargin{:});


This is beautiful. So clean and easy on the eyes!

gchenfc added 5 commits December 8, 2021 19:47

fix unit test for DefaultFuncInt only (others not fixed yet)

5e2aa15

placeholder folder for unit test "actual" folder

b9409f1

Update class_wrapper matlab unit test

b7e4d35

class wrapper working

4a40032

update functions.i unit test expected and fix cyclic copy

6166dfa

gchenfc requested a review from varunagrawal December 9, 2021 05:32

gchenfc linked an issue Dec 9, 2021 that may be closed by this pull request

Matlab default arguments #134

Closed

gchenfc added feature New feature or request matlab labels Dec 9, 2021

gchenfc requested a review from mattking-smith December 9, 2021 06:01

varunagrawal reviewed Dec 9, 2021

View reviewed changes

mattking-smith requested changes Dec 9, 2021

View reviewed changes

mattking-smith approved these changes Dec 9, 2021

View reviewed changes

take out unit test with illegal default argument specification (accor…

702986d

…ding to c++ rules)

don't allow out-of-order default argument specifications in matlab

d3a0a8a

gchenfc requested a review from varunagrawal December 10, 2021 04:24

varunagrawal approved these changes Dec 13, 2021

View reviewed changes

varunagrawal merged commit 086be59 into master Dec 13, 2021

varunagrawal deleted the feature/matlab_default_args branch December 13, 2021 13:52

Matlab default arguments #135

Matlab default arguments #135

Conversation

gchenfc commented Dec 9, 2021 • edited Loading

gchenfc commented Dec 9, 2021

mattking-smith commented Dec 9, 2021

varunagrawal commented Dec 9, 2021

varunagrawal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchenfc Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchenfc commented Dec 9, 2021

mattking-smith left a comment • edited Loading

Choose a reason for hiding this comment

gchenfc commented Dec 9, 2021

varunagrawal commented Dec 9, 2021

gchenfc commented Dec 9, 2021

mattking-smith commented Dec 9, 2021

varunagrawal commented Dec 9, 2021

mattking-smith commented Dec 9, 2021

mattking-smith commented Dec 9, 2021

mattking-smith left a comment

Choose a reason for hiding this comment

mattking-smith commented Dec 9, 2021

varunagrawal commented Dec 9, 2021

mattking-smith commented Dec 9, 2021

varunagrawal commented Dec 10, 2021

mattking-smith commented Dec 10, 2021

mattking-smith commented Dec 12, 2021

varunagrawal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchenfc commented Dec 9, 2021 •

edited

Loading

gchenfc Dec 9, 2021 •

edited

Loading

mattking-smith left a comment •

edited

Loading