Order of keys in Expressions

[Jacob Thomson]

Keys are stored in an std::set for Expressions. This reorders the keys and can make use of them quite misleading.

ExpressionFactor2::expression() makes this mistake and assumes that the expressions are in the correct order:

return expression(this->keys_[0], this->keys_[1]);

This is not always the case and leads to incorrect serialization.

Perhaps the keys could instead be stored in a vector?

[Duy-Nguyen Ta]

I've tried to address this issue in the pull request #240. Can someone have a look?

[Jacob Thomson]

Hmm thanks, my change was a bit more intrusive, i.e. changing Expression keys() to use a FastVector<Key> instead of a set, which makes more sense to me but perhaps I'm missing something.

A comment in Expression.h, "The order of the Jacobians is the same as keys" implies there was some thought behind it.

[Jacob Thomson]

Had some more time to look at this, I think the set is used to group duplicate keys. Pull request #240 wouldn't work so well if you had a ExpressionFactor2 with two of the same keys. I created an alternative in pull request #269 that explicitly stores the keys and serializes them. Perhaps a more generic way would be to create a:

FastVector<Key> Expression::orderedKeys();

That could be called on serialization and then unpacked for deserialization.

[Frank Dellaert]

I think this is fixed with PR #240