add Sim(3)-based alignment to the wrapper

[johnwlambert]

• Fix pose notation in Sim(3) Align() code and unit tests
• Add more docstrings
• Move Sim(3) out of unstable to stable GTSAM
• Add Sim(3) to python wrapper
• Add python unit tests

3rd python unit test checks alignment here between 4 pose-pairs:

[ayushbaid]

Hi John. Is this algorithm the same as what we are trying to implement in GTSFM?

[johnwlambert]

Hi John. Is this algorithm the same as what we are trying to implement in GTSFM?

Yes, although PosePairs (wTi, wTi_) from wTi_list, wTi_list_ is the one that was suggested: https://github.com/borglab/gtsam/blob/develop/gtsam_unstable/geometry/Similarity3.cpp#L170

[akshay-krishnan]

I think we should also add some unit tests for Similarity3 if there are not any already, especially since its being taken out of unstable.

[johnwlambert]

I think we should also add some unit tests for Similarity3 if there are not any already, especially since its being taken out of unstable.

Agreed, yeah we should do that

[akshay-krishnan]

It looks like there is a testSimilarity3 in unstable which broke because we moved the file. Maybe we can try moving that as well, and that might fix the CI?

[johnwlambert]

It looks like there is a testSimilarity3 in unstable which broke because we moved the file. Maybe we can try moving that as well, and that might fix the CI?

Good point. But also seems like PointPairs isn't making its way into the gtsam namespace, also:

/home/runner/work/gtsam/gtsam/build/python/gtsam.cpp: In function ‘void pybind11_init_gtsam(pybind11::module&)’:
/home/runner/work/gtsam/gtsam/build/python/gtsam.cpp:1163:45: error: ‘PointPairs’ in namespace ‘gtsam’ does not name a type
         .def_static("Align",[](const gtsam::PointPairs& abPointPairs){return gtsam::Similarity3::Align(abPointPairs);}, py::arg("abPointPairs"));
                                             ^
/home/runner/work/gtsam/gtsam/build/python/gtsam.cpp:1163:129: error: expected ‘)’ before string constant
         .def_static("Align",[](const gtsam::PointPairs& abPointPairs){return gtsam::Similarity3::Align(abPointPairs);}, py::arg("abPointPairs"));

[varunagrawal]

Looks good, but please rename PointPairs to Point3Pairs and use the preferred using idiom for typedefs. Also @johnwlambert be sure to link your commits to your account.

[asa]

Using is more “modern”.

…

On Thu, Feb 25, 2021 at 7:27 PM Varun Agrawal ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In gtsam/geometry/Similarity3.h <#702 (comment)>: > @@ -30,6 +30,8 @@ namespace gtsam { // Forward declarations class Pose3; +typedef std::vector<Point3Pair> PointPairs; https://stackoverflow.com/a/23196675/1236990 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#702 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAXPGOHJVBVPGTWYWZKSATTA4IJXANCNFSM4YFIQFRQ> .

[varunagrawal]

LGTM with some minor changes. 🙂

[dellaert]

Hmmmm, CI fails, but it seems with a compiler error, and only on Linux :-(

[dellaert]

(Other than that LGTM, but that needs to be investigated before we can merge)

[johnwlambert]

(Other than that LGTM, but that needs to be investigated before we can merge)

Yeah, @ProfFan thought it was likely OOM issue from the large gtsam.i file?

[dellaert]

Definitely possible. @jingwuOUO reported similar issues on her Linux box. @ProfFan and @varunagrawal what can we do to split up gtsam.i? One file per subdir?

[varunagrawal]

I don't think we even need that. We need to figure out a way to break down the gtsam.cpp (which is fed to Pybind) into smaller files that can be parallelly built. At the end of the day, it generates one shared object file so that would be the easiest way to do it.

[jingwuOUO]

When make -j1 python-install, it often causes my computer freezing and raise error g++-7: internal compiler error: Killed (program cc1plus), my processor is i7-8550U @ 1.80GHz x 8.

[varunagrawal]

What happens when you run make -j8?

I agree that the Pybind11 wrapper is a major memory hog. I had to update my swap from 8 GB to 24 GB to prevent my laptop from freezing as well.

[jingwuOUO]

Tried, it will crash sooner. The same error message.

[jingwuOUO]

Oh my swap is 8GB. I might need to update.

[ProfFan]

Swapping makes compilation extremely slow (the compiler really does not like the disk). Splitting the header is the fastest way of cutting down compilation time and memory, since splitting gtsam.cpp is not an easy job (dependency analysis, etc. etc.).

[johnwlambert]

CI passes now that TBB + Python is disabled in the CI

discussed changes have been made

[varunagrawal]

Interesting. I wonder if TBB is doing something to cause the OOM (instead of the wrapper). We need to update the TBB dependent code to use the new API.