Skip to content

Conversation

@plusk01
Copy link
Contributor

@plusk01 plusk01 commented Jan 29, 2023

mainly minor, readability changes

  • Update to GTSAM 4.2a8 - pinning to a tagged version is perhaps better than a specific commit (required no changes)
  • update flake8 url for pre-commit
  • update README
  • clean up CMakeLists.txt to conform with modern standards
  • uses FetchContent cmake module to include matplotlibcpp as external project (cmake will clone the repo so the user doesn't have to install it globally)
  • clean up plotting in tests
  • remove using namespace dcsam from test -- being explicit makes it easier for a new user to differentiate between dcsam and gtsam APIs

@keevindoherty
Copy link
Collaborator

Amazing work on this :) Thanks @plusk01

I think GTSAM 4.2a8 also introduces some new syntax for hybrid graphs. I'm not sure how ready for prime time that stuff is, but could be worthwhile to take a look at in the future.

Regarding syncing DC-SAM to the latest GTSAM release, I think the plan will be to cut a release branch off main (now that your normalizing constant fix is merged) and then make your branch the new main. This way "legacy" code that depends on the current "release" (using these terms very loosely) and therefore the older GTSAM version will still be relatively usable.

@plusk01
Copy link
Contributor Author

plusk01 commented Jan 30, 2023

makes sense - is it worth revisiting #2 and #26 before making the release/gtsam4.1.1 branch? I didn't look at their contents too closely, but the PR titles sound useful :)

@keevindoherty
Copy link
Collaborator

@plusk01 Those are a bit stale at the moment. I'm comfortable closing the sum-mixtures line (never quite worked in tests).

As for the factor removal, I think we can pull in the latest from the 4.2a8-synced main branch to the factor removal branch if we want to continue development on that line. I agree it is a useful feature.

@keevindoherty keevindoherty merged commit 6fdca48 into MarineRoboticsGroup:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants