Some Cleanup

[varunagrawal]

Had some pending fixes to clean up. No code changes per se.

[johnwlambert]

thanks for the PR, Varun. Can you explain this change a bit more? We use this code in GTSFM as:

        # Generate the DSF to form tracks
        dsf = gtsam.DSFMapIndexPair()
        track_2d_list = []
        # for DSF finally
        # measurement_idxs represented by ks
        for (i1, i2), k_pairs in matches_dict.items():
            for (k1, k2) in k_pairs:
                dsf.merge(gtsam.IndexPair(i1, k1), gtsam.IndexPair(i2, k2))

        key_set = dsf.sets()

        erroneous_track_count = 0
        # create a landmark map: a list of tracks
        # Each track is represented as a list of (camera_idx, measurements)
        for set_id in key_set:
            index_pair_set = key_set[set_id]  # key_set is a wrapped C++ map, so this unusual syntax is required

            # Initialize track from measurements
            track_measurements = []
            for index_pair in gtsam.IndexPairSetAsArray(index_pair_set):

[johnwlambert]

so i believe this would be a breaking change. I'm guessing the new syntax would be cleaner, but I'm just wondering what changes we would need to make.

[dellaert]

I think we should not merge until @johnwlambert asserts GTSFM will work.

[varunagrawal]

This should not break anything since I simply uncommented the templated version of this function a few lines above. The syntax in the wrapper should be identical.

[ProfFan]

Maybe just the name will be different? @johnwlambert Could you test if this is the case or not?

[johnwlambert]

Ah I see @varunagrawal, I had forgotten that the wrapper will concatenate the template type to the end of the name, so the name should be unchanged.

To make sure the functionality is the same as before, I expanded some unit tests in this other PR: https://github.com/borglab/gtsam/pull/854/files

If #854 looks ok to you, maybe we can merge it into develop, then pull develop into this branch, and as long as all the tests still pass, feel free to merge.

[varunagrawal]

Done and done.

[dellaert]

I think we should not merge until @johnwlambert asserts GTSFM will work.

[varunagrawal]

Just waiting on @dellaert and we can merge.

[berndpfrommer]

I'm getting errors from the Ubuntu build server for xenial (Ubuntu 16.04) that could be coming from this change:

-- Configuring done
CMake Error at CppUnitLite/CMakeLists.txt:6 (add_library):
  Target "CppUnitLite" links to target "Boost::boost" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

Note that Ubuntu 16.04 ships with libboost v1.58.
Everything works seems to work fine on Ubuntu 18.04 and 20.04.
Is it the intention to stop supporting Ubuntu 16.04? If yes then I will disable nightly building for that release.

[varunagrawal]

Can we instead configure the build server to use Boost 1.65 or 1.67 at a minimum? That's the only difference that this PR introduces in terms of build.

[berndpfrommer]

Anything later than boost v 1.58 will have to come from another ppa. So yes, this can be done, but xenial users will then be forced to remove the native libboost 1.58 from their system and install 1.65 from a PPA before they can install gtsam. For most people removing libboost 1.58 will be a non-starter because it will require removal of a potentially very long list of packages that depend on it.

[dellaert]

Hmmm - mostly silent on this thread but I do think Boost version upgrades are a big deal, @varunagrawal . Is there a very strong argument as why we should have 1.65 over 1.58?

[dellaert]

Specifically, what new functionality or bugfix is needed from 1.65?

[varunagrawal]

1.65 solves some issues with serialization that others and myself have faced. Specifically, prior to 1.65, Boost doesn't perform deep serialization reliably (shallow serialization works great).

[dellaert]

Hmmm, what does "reliably" mean in your reply? Does it work, or not? Or are you saying it is non-deterministic? Or does it work for most things, but one specific structure is known to fail?

Not opposed to upgrading requirements to Ubuntu > 18.04 (Bernd, what is the default installed Boost version there?) but we should do it carefully and based on exact information.

[berndpfrommer]

Ubuntu 16.04: boost 1.58
Ubuntu 18.04: boost 1.65
Ubuntu 20.04: boost 1.66

There are still some users running 16.04, I suspect on older robots that they don't want to upgrade. Ubuntu 18.04 is still very common. For example NVidia doesn't even support 20.04 yet on some of their platforms.

[berndpfrommer]

In fact, let me back paddle hard on my statement that I can install a PPA on a Ubuntu build server. I can only install this with our nightly packaging builds, but not on the Ubuntu build servers. So in fact to make this work we'd have to package boost along with gtsam which is probably not an option ever. Sorry about the confusion.

[ProfFan]

1.58 has a known bug that prevents proper serialization and AFAIK ubuntu is not doing backport patches for boost...

Still I think we do not have anything intentional that prevents you from building with 1.58. But it is not in the CI because of the aforementioned serialization failure.

[dellaert]

In that case, perhaps best course of action is:

• Document in README or INSTALL why we require 1.65 in general (link to bug)
• Allow Linux to compile with 1.58, but print out a warning referring to above

Since this PR is the one breaking 16.04, I think it's on @varunagrawal to quickly write an amendement :-)

[varunagrawal]

We have a whole issue about this bug in Boost. 😄 #590

[dellaert]

We have a whole issue about this bug in Boost. 😄 #590

Excellent. Let our users know about it by documenting it.

[varunagrawal]

It's already documented in the INSTALL.md under Known Issues.