Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/factor removal #2

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Feature/factor removal #2

wants to merge 9 commits into from

Conversation

kurransingh
Copy link
Contributor

Augmented "update" method to pass through to the isam and DiscreteFactorGraph removal methods. We leave it up to the user to keep track of factor indices.

Comment on lines 1545 to 1548
dcsam.update(hfg, initialGuess, initialGuessDiscrete, removals, discreteRemovals);

EXPECT_EQ(dcsam.getDiscreteFactorGraph().at(17), nullptr);
EXPECT_EQ(dcsam.getNonlinearFactorGraph().at(17), nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't validate the new estimate after the factor removal.

Suggestion: Try to solve a (simple) factor graph with an known solution (that we can verify to within tol). Then, remove one or more factors, verify with EXPECT_EQ that they've been removed, retrieve the new estimate via dcsam_.calculateEstimate() and validate that the new estimate is correct (to within tol).

There are also a couple "edge cases" we might want to check on, e.g. what happens if we have a discrete variable with a single discrete factor attached to it, and then we remove that factor? I'm not actually sure what the behavior would be in this case, but it seems like it would be good to know.

Copy link
Contributor Author

@kurransingh kurransingh Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we have a discrete variable with a single discrete factor attached to it, and then we remove that factor?

The variable will be removed (I believe using the gtsam::VariableIndex that is computed), and any subsequent attempts to retrieve information about the variable will throw errors.

tests/testDCSAM.cpp Outdated Show resolved Hide resolved
include/dcsam/DCSAM.h Outdated Show resolved Hide resolved
@keevindoherty
Copy link
Collaborator

Just checking in on this - is there anything outstanding on this PR preventing it from being merged?

@keevindoherty
Copy link
Collaborator

@kurransingh Do you remember where we left this off? The main branch is now synced to GTSAM 4.2a8, so if we wanted to push further on this, just need to make sure we pull in those changes first.

This seems like a useful feature, but want to make sure everything is working correctly.

@kurransingh
Copy link
Contributor Author

Hey sorry for the extremely late response on this @keevindoherty. I've pulled in all of the latest changes etc. and everything seems to be working. Good to merge from my perspective.

Copy link
Collaborator

@keevindoherty keevindoherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. I haven't tested anything on my end but happy to trust that this is working based on your own usage internal to the MRG. There were a couple TODOs in comments and I wasn't sure if they were stale, so I marked those for your input, but pending that I'm happy to merge!

Thanks for revisiting this, by the way! I know it's been a while and it kind of fell off my radar.

Comment on lines +170 to 171
// NOTE: I am not yet 100% sure this is the right way to handle this update.
isam_.update(newFactors, initialGuess, updateParams);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurransingh Is this comment still valid?

// make sure continuous removal works as well
gtsam::FactorIndices removals{5};

// TODO(Kurran) why does removing continous factor 5 cause isam segfault?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurransingh Is this comment stale?

size_t mpeClassL1 = dcvals.discrete.at(lc1);
EXPECT_EQ(mpeClassL1, 1);

std::cout << "now let's remove factors! " << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this print needed?

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.

3 participants