Gaussian Factor Graph Marginals #139
Conversation
(with Python bindings)
|
|
||
| /* ************************************************************************* */ | ||
| Marginals::Marginals(const GaussianFactorGraph& graph, const VectorValues& solution, Factorization factorization, | ||
| EliminateableFactorGraph<GaussianFactorGraph>::OptionalOrdering ordering) |
There was a problem hiding this comment.
Let's just use boost::optional<Ordering> everywhere? Including in .h file?
gtsam/nonlinear/Marginals.cpp
Outdated
| : graph_(graph), factorization_(factorization) { | ||
| gttic(MarginalsConstructor); | ||
| Values vals; | ||
| for (const VectorValues::KeyValuePair& v: solution) { |
There was a problem hiding this comment.
Let's modernize this to for (const auto& keyValue: solution) here and elsewhere in this file?
|
@ProfFan what's the deal here. Should it be 'const boost::optional&` or boost::optional<const Ordering&>'. I think we ran into some issues with the latter for some compiler, when the Ordering passed in is a temporary object. |
|
Yeah, I know... Questioning that "wisdom". |
|
I know this is a longshot, but as Jose mentioned, moving to C++17 would be a great idea, since it would allow us to use the native Just something to keep in mind. 🙂 |
|
Not doing that yet. |
|
@dellaert Today I encountered one problem debugging for Yetong. In some of the dynamics code we are passing temporary objects to EDIT: Previous versions of boost will not complain, but anyway this require the temporary object to be stored in a variable instead, e.g. ORIG: NEW |
|
Exactly. The issue then is, if we say 'const boost::optional&`, is this efficient? Is a copy being made? And will it behave correctly for temporaries (e.g., and Ordering is constructed as part of the argument list) |
|
For size: #include <iostream>
#include <memory>
#include <string>
#include <boost/optional.hpp>
class BigClass {
public:
int bigArray[2333];
BigClass(int a0){
bigArray[0] = a0;
}
};
int main() {
auto test = "Squat TEST!";
std::unique_ptr<BigClass> ptr(new BigClass(0));
boost::optional<BigClass> opt_empty;
boost::optional<BigClass> opt_full(0);
std::cout << "ptr: " << sizeof(ptr) << '\n';
std::cout << "*ptr: " << sizeof(*ptr) << '\n';
std::cout << "opt_empty: <
8000
span class="pl-pds">" << sizeof(opt_empty) << '\n';
std::cout << "opt_full: " << sizeof(opt_full) << '\n';
std::cout << "*opt_full: " << sizeof(*opt_full) << '\n';
return 0;
}> gcc test.cpp -lstdc++ -o bin && ./bin
ptr: 8
*ptr: 9332
opt_empty: 9336
opt_full: 9336
*opt_full: 9332 |
|
In terms of efficiency, if we are converting an ordinary object to an optional, there will always be a copy, as the layout of |
|
After all, to me, simple overloading seems to be the best approach to optional parameters. (Remember hearing that in a CppCon presentation): https://foonathan.net/2018/07/optional-reference/ |
|
Wow. Nice experimenting. I see the problem, but I think in this case overloading won’t work as we’re passing optional down.. Without copies, I think, these are const refs. Here is what I propose, @gchenfc : please change all to @ProfFan ? |
|
I suspect this may be difficult to change without changing more code first, I'm getting
but may just be my lack of proficiency around boost/templating, I'm trying |
|
perhaps changing back to |
|
Yes
On Thu, Oct 10, 2019 at 00:57 Gerry Chen ***@***.***> wrote:
perhaps changing back to
EliminateableFactorGraph<GaussianFactorGraph>::OptionalOrdering ordering
and then doing a separate PR to change OptionalOrdering typedef?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#139?email_source=notifications&email_token=ACQHGSMOGZTYXEZIQZCDOBLQN2Y3PA5CNFSM4I7EGIL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA2UE4Y#issuecomment-540361331>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACQHGSO3BWGYQHO6IV4PVI3QN2Y3PANCNFSM4I7EGILQ>
.
--
Best !
Frank Dellaert
http://frank.dellaert.com
|
|
changed back, compiled and passed tests on my computer. |
|
@dellaert Judging from the semantics, I think we can have an overload here between |
2cbaf7a3a Merge pull request #131 from borglab/remove-pickle 0c2a4483c add documentation 2e5af11ad Merge pull request #139 from borglab/fix/matlab-memory-leak 442b7d3b0 update test fixtures f89d5e4bd delete object after clearing it from object collector 971282703 add the correct variable for RTTI registry creation 9758dec57 update test fixture 87aeb8f8c remove need for declaring pickle function in interface file git-subtree-dir: wrap git-subtree-split: 2cbaf7a3a628766ff40657e0150b407ed4af7ccd
Currently Marginals class only has a constructor from a nonlinear factor graph. Adding a constructor for a linear factor graph is trivial by just skipping the linearization step.
updated c++ unittests and created a python unittest, all pass
My c++ is not strong so let me know if I am doing some silly things.
This change is