Expose some iSAM2/FixedLagSmoothing functionality for the bindings #1053

senselessDev · 2022-01-22T14:49:11Z

I was not able to use the findUnusedFactorSlots parameter from Python (compare with #150).
Also a new overloaded update method for the FixedLagSmoother allows to give factors that shall be removed.
And last but not least, IncrementalFixedLagSmoother exposes the internal factor graph.

dellaert · 2022-01-22T15:34:22Z

Cool. Will run CI after #1054 is merged. Could you maybe add a unit test in python/gtsam_unstable/tests ? You could do this using a paired down version of test_FixedLagSmootherExample.py.

Finally, @varunagrawal and @ProfFan : can't we now just access fields in a struct? This would have to work in matlab and python, of course.

varunagrawal · 2022-01-22T15:44:59Z

gtsam_unstable/gtsam_unstable.i

@@ -567,6 +567,10 @@ virtual class FixedLagSmoother {
  double smootherLag() const;

  gtsam::FixedLagSmootherResult update(const gtsam::NonlinearFactorGraph& newFactors, const gtsam::Values& newTheta, const gtsam::FixedLagSmootherKeyTimestampMap& timestamps);
+  gtsam::FixedLagSmootherResult update(const gtsam::NonlinearFactorGraph &newFactors,


You can just update the above update by adding the extra parameter and specifying a default parameter.

varunagrawal · 2022-01-22T15:45:52Z

Yes! Accessing member variables in classes and structs is supported.

varunagrawal · 2022-01-22T15:46:16Z

And when I say supported, I mean across all languages. 🙂

dellaert · 2022-01-22T16:03:23Z

And when I say supported, I mean across all languages. 🙂

Cool, so, could you comment on how @senselessDev would change the PR; the c++ stubs would no longer be needed I guess.

senselessDev · 2022-01-22T17:30:22Z

Ahh, nice to know. I am more or less familiar with pybind and I presume you are talking about this: https://pybind11.readthedocs.io/en/stable/classes.html#instance-and-static-fields
Especially, about def_readwrite.

I have found it in the generated binding code only a few times, but I think for example it is being generate by

gtsam/gtsam/slam/slam.i

Lines 217 to 219 in 0638727

    
           double r; 
        
           double g; 
        
           double b;

Right?

senselessDev · 2022-01-22T17:59:30Z

Just tried it out in bcbd26c, is working for me:

In [1]: import gtsam

In [2]: p = gtsam.ISAM2Params()

In [3]: p
Out[3]: 

type:              ISAM2GaussNewtonParams
wildfireThreshold: 0.001
relinearizeThreshold:              0.1
relinearizeSkip:                   10
enableRelinearization:             1
evaluateNonlinearError:            0
factorization:                     CHOLESKY
cacheLinearizedFactors:            1
enableDetailedResults:             0
enablePartialRelinearizationCheck: 0
findUnusedFactorSlots:             0

In [4]: p.findUnusedFactorSlots = True

In [5]: p
Out[5]: 

type:              ISAM2GaussNewtonParams
wildfireThreshold: 0.001
relinearizeThreshold:              0.1
relinearizeSkip:                   10
enableRelinearization:             1
evaluateNonlinearError:            0
factorization:                     CHOLESKY
cacheLinearizedFactors:            1
enableDetailedResults:             0
enablePartialRelinearizationCheck: 0
findUnusedFactorSlots:             1

dellaert · 2022-01-22T18:02:19Z

Cool! But please run make check, make python-install and make python-test and fix any issues? This is an API change, which I am generally OK with in a new minor version release if the code is rarely used, but I'd like to know you did the check before I run CI.

senselessDev · 2022-01-22T19:41:37Z

@varunagrawal
Hmm, using the default parameter for FactorIndices is not so easy, because it is ignored in https://github.com/borglab/gtsam/blob/develop/python/CMakeLists.txt#L37

Hence, on importing gtsam_unstable it is failing with

----> 1 from .gtsam_unstable import *

ImportError: arg(): could not convert default argument into a Python object (type not registered yet?). Compile in debug mode for more information.

One idea is to include #include <pybind11/stl.h> in https://github.com/borglab/gtsam/blob/develop/python/gtsam_unstable/preamble.h which would make it very comfortable to just use python lists for any FactorIndices.
But that currently fails with gtsam::SimWall2DVector, gtsam::SimPolygon2DVector and gtsam::FixedLagSmootherKeyTimestampMap currently...

senselessDev · 2022-01-22T21:54:23Z

decide how to proceed with default parameter and FactorIndices
remove set.. warppers in all the examples (matlab & python)
run tests (I can't test matlab)

dellaert · 2022-01-22T22:18:44Z

FactorIndices is preamble seems like a good solution. Why does it not work?
removing all set/get in wrapper is probably for a different PR
run tests, definitely :-) @mattking-smith might be able to run the tests in MATLAB from this branch?

varunagrawal · 2022-01-24T18:15:50Z

gtsam/nonlinear/ISAM2Params.h

@@ -300,50 +300,24 @@ struct GTSAM_EXPORT ISAM2Params {
  RelinearizationThreshold getRelinearizeThreshold() const {
    return relinearizeThreshold;
  }
-  int getRelinearizeSkip() const { return relinearizeSkip; }


Why are we removing all these methods? Isn't this API breaking and tangential to the point of this PR?

Why are we removing all these methods? Isn't this API breaking and tangential to the point of this PR?

It'll help if you read my review comments as well....

varunagrawal · 2022-01-24T18:18:43Z

gtsam/nonlinear/nonlinear.i

-  void setEnableDetailedResults(bool enableDetailedResults);
-  bool isEnablePartialRelinearizationCheck() const;
-  void setEnablePartialRelinearizationCheck(
-      bool enablePartialRelinearizationCheck);


Again, you don't need to remove these methods to add the variable access. This would break the API for other users.

varunagrawal · 2022-01-24T18:28:36Z

Did a quick run through while eating lunch: I am not sure I agree with the methods being removed. Sometimes, you just want to pass functionals to other methods/functionals thus the point of setters and getters in a hybrid functional programming paradigm (which is what we have).

Also, please please please add some python unit tests for the changes you have made. That way I can help you figure out exactly what you need since if the tests reflect your desired behavior, my suggestions will align with your end goal. :)

dellaert · 2022-01-24T18:40:07Z

Did a quick run through while eating lunch: I am not sure I agree with the methods being removed. Sometimes, you just want to pass functionals to other methods/functionals thus the point of setters and getters in a hybrid functional programming paradigm (which is what we have).

Also, please please please add some python unit tests for the changes you have made. That way I can help you figure out exactly what you need since if the tests reflect your desired behavior, my suggestions will align with your end goal. :)

Agree with unit test comment.

But the comment about "Sometimes,..." seems a bit arcane. I'd opt for KISS. These methods were in actuality only added for the wrapper. I'd as soon remove them. Less code to maintain.

dellaert · 2022-01-24T18:41:34Z

Also: "This is an API change, which I am generally OK with in a new minor version release if the code is rarely used". I think this is the case here. And more is coming down the pipe.

varunagrawal · 2022-01-24T18:52:23Z

If we're all on the same page about API changes then I am happy :)

Also regarding the import error, adding pybind/stl.h is something I've been meaning to do forever, but I only have so much time on my hands. For now, you can just set the default to std::vector<gtsam::FactorIndex>(). What happens here is that this is read by the C++ compiler and the default argument is generated accordingly (since FactorIndices is just an alias to std::vector<gtsam::FactorIndex>()).

varunagrawal · 2022-01-24T19:23:48Z

Okay nvm my last comment. You can go back to having two versions of wrapped update until we can figure out the stl bindings.

dellaert · 2022-01-24T19:26:14Z

If we're all on the same page about API changes then I am happy :)

Also regarding the import error, adding pybind/stl.h is something I've been meaning to do forever, but I only have so much time on my hands. For now, you can just set the default to std::vector<gtsam::FactorIndex>(). What happens here is that this is read by the C++ compiler and the default argument is generated accordingly (since FactorIndices is just an alias to std::vector<gtsam::FactorIndex>()).

I thought this was already supported. I definitely added, say, DiscreteKeys to a preamble so it behaves just like a dict in python...

varunagrawal · 2022-01-24T20:03:30Z

Yes but FactorIndices is weird, it is an alias of vector<Key> which is already defined in preamble/gtsam.h so there is a collision there, and even adding it is not making a dent.

varunagrawal · 2022-01-24T20:06:25Z

This is primarily because FactorIndices is a typedef while DiscreteKeys is a subclass.

senselessDev · 2022-01-24T20:47:44Z

So, to summarize, we want to get rid of all those setters/getters when an equivalent direct member access is possible. And I need to change that in all the tests/examples which use it. Right?

Regarding subclassing and typedef, I am not too sure. I played around with the FixedLagSmootherKeyTimestampMap which I was able to use the opaque automatic binding allowing m[1] = 0.0 etc. And this one also is only a typedef. But with this vector I had no success as well. Maybe it is really an issue with both libs. From the pybind docs:

This macro must be specified at the top level (and outside of any namespaces), since it adds a template instantiation of type_caster. If your binding code consists of multiple compilation units, it must be present in every file (typically via a common header) preceding any usage of std::vector

I have not tried it yet with defining them in both.

BTW, I think I have discovered an issue that the specialization header of gtsam_unstable does not reflect changes properly. It is copied into the build directories only once but not every time when the file in the source directory changes. This costed me a lot of time until discovered. I had no motivation left to see how to trigger that via CMake. Maybe that does help. Maybe it was only a very weird problem on my machine. Just in case you are wondering, maybe that's an explanation.

dellaert · 2022-01-24T23:17:57Z

Yes, feel free get rid of setters/getters in the one class (let's keep this PR specific) except the ones that examine a string argument. If you do so, then indeed tests and examples have to be fixed or CI will not pass.

cmake issue: I know, I have been frustrated by this before. @varunagrawal or @ProfFan might be able to look at this after the RSS deadline.

dellaert · 2022-01-28T02:47:48Z

Can you comment on whether make check and make python-test work? Then I'll trigger CI and we can merge.

senselessDev · 2022-01-28T06:35:56Z

Sorry, I had no time to implement all changes we discussed. Hopefully sometime during this weekend..

* FactorIndices default argument is currently not easily available in binding code * see borglab#1053 (comment)

senselessDev · 2022-01-29T22:45:24Z

Pushed now the adapted examples with direct member access. Tested with make check, make python-test, all still working. Little extra stuff is additionally exposed and the __repr__ printing of the IncrementalFixedLagSmoother and BatchFixedLagSmoother is fixed.

What I did not do (and can not) is matlab testing.

dellaert · 2022-01-29T22:54:34Z

@mattking-smith is this something you can test before we merge?

dellaert

This looks good to me! But I prefer to wait with merging until we know about the MATLAB implication. @varunagrawal , should it work, in theory? @mattking-smith does it work, in practice?

varunagrawal · 2022-01-30T18:26:18Z

I can run the Matlab tests in a bit and give you a definite answer. :)

varunagrawal · 2022-02-01T20:55:27Z

I am happy to report that all Matlab tests pass. 🙂 I'll go ahead and merge this.

dellaert · 2022-02-01T21:43:25Z

Thanks @senselessDev !!!!

dellaert · 2022-02-02T21:22:12Z

@mattking-smith FYI

expose some iSAM2/FixedLagSmoothing functionality for the bindings

0814efa

varunagrawal reviewed Jan 22, 2022

View reviewed changes

use direct memeber access for ISAM2Params

bcbd26c

use default for parameter instead of overloading

3053008

senselessDev mentioned this pull request Jan 23, 2022

Expose GraphvizFormatting and test in python #1059

Merged

varunagrawal reviewed Jan 24, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into develop

2a17280

Merge remote-tracking branch 'upstream/develop' into develop

bd4effb

senselessDev added 6 commits January 29, 2022 22:03

Merge remote-tracking branch 'upstream/develop' into develop

a82ddcc

revert FactorIndices default argument to overloading for now

dbfc7bb

* FactorIndices default argument is currently not easily available in binding code * see borglab#1053 (comment)

remove last overseen stub for bindings

11b2135

adapt examples to new direct member access

65d72ab

expose iSAM2 parameter factorization enum

40d3bad

expose iSAM object for iFLS, fix __repr__ of iFLS, bFLS

7526329

dellaert approved these changes Jan 29, 2022

View reviewed changes

varunagrawal merged commit ad0d7e4 into borglab:develop Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose some iSAM2/FixedLagSmoothing functionality for the bindings #1053

Expose some iSAM2/FixedLagSmoothing functionality for the bindings #1053

senselessDev commented Jan 22, 2022

dellaert commented Jan 22, 2022

varunagrawal Jan 22, 2022

varunagrawal commented Jan 22, 2022

varunagrawal commented Jan 22, 2022

dellaert commented Jan 22, 2022

senselessDev commented Jan 22, 2022 •

edited

Loading

senselessDev commented Jan 22, 2022

dellaert commented Jan 22, 2022

senselessDev commented Jan 22, 2022 •

edited

Loading

senselessDev commented Jan 22, 2022 •

edited

Loading

dellaert commented Jan 22, 2022

varunagrawal Jan 24, 2022

dellaert Jan 24, 2022

varunagrawal Jan 24, 2022

varunagrawal commented Jan 24, 2022

dellaert commented Jan 24, 2022

dellaert commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

dellaert commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

senselessDev commented Jan 24, 2022 •

edited

Loading

dellaert commented Jan 24, 2022

dellaert commented Jan 28, 2022

senselessDev commented Jan 28, 2022

senselessDev commented Jan 29, 2022

dellaert commented Jan 29, 2022

dellaert left a comment

varunagrawal commented Jan 30, 2022

varunagrawal commented Feb 1, 2022

dellaert commented Feb 1, 2022

dellaert commented Feb 2, 2022

Expose some iSAM2/FixedLagSmoothing functionality for the bindings #1053

Expose some iSAM2/FixedLagSmoothing functionality for the bindings #1053

Conversation

senselessDev commented Jan 22, 2022

dellaert commented Jan 22, 2022

varunagrawal Jan 22, 2022

Choose a reason for hiding this comment

varunagrawal commented Jan 22, 2022

varunagrawal commented Jan 22, 2022

dellaert commented Jan 22, 2022

senselessDev commented Jan 22, 2022 • edited Loading

senselessDev commented Jan 22, 2022

dellaert commented Jan 22, 2022

senselessDev commented Jan 22, 2022 • edited Loading

senselessDev commented Jan 22, 2022 • edited Loading

dellaert commented Jan 22, 2022

varunagrawal Jan 24, 2022

Choose a reason for hiding this comment

dellaert Jan 24, 2022

Choose a reason for hiding this comment

varunagrawal Jan 24, 2022

Choose a reason for hiding this comment

varunagrawal commented Jan 24, 2022

dellaert commented Jan 24, 2022

dellaert commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

dellaert commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

varunagrawal commented Jan 24, 2022

senselessDev commented Jan 24, 2022 • edited Loading

dellaert commented Jan 24, 2022

dellaert commented Jan 28, 2022

senselessDev commented Jan 28, 2022

senselessDev commented Jan 29, 2022

dellaert commented Jan 29, 2022

dellaert left a comment

Choose a reason for hiding this comment

varunagrawal commented Jan 30, 2022

varunagrawal commented Feb 1, 2022

dellaert commented Feb 1, 2022

dellaert commented Feb 2, 2022

senselessDev commented Jan 22, 2022 •

edited

Loading

senselessDev commented Jan 22, 2022 •

edited

Loading

senselessDev commented Jan 22, 2022 •

edited

Loading

senselessDev commented Jan 24, 2022 •

edited

Loading