-
Notifications
You must be signed in to change notification settings - Fork 4
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
Partial support of the sparse matrices in roboptim-core-manifold. #7
Conversation
} | ||
for (int i = 0; i < jac.rows(); ++i) | ||
{ | ||
std::cout << jac.row(i) << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why print to std::cout
like this rather than doing the usual std::cout << output->str () << std::endl;
at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the update
Do you explicitly check for the assumption you're making? |
"the manifold is " << descWrap.manifold().name() << "\n" << | ||
"Is the manifold elementary ? : " << | ||
(descWrap.manifold().isElementary() ? "yes" : "no" ) << "\n" << | ||
"the manifold dimension is " << descWrap.manifold().representationDim() << "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note (even though that's not the focus of this PR): for better printing, RobOptim provides some handy indentation support. Also, to be even pickier, whitespace before punctuation is not valid in English.
Ok about the exception throwing and type checking. I updated the pull request taking considering that. |
iendl << "Is the manifold elementary? " << | ||
(descWrap.manifold().isElementary() ? "yes" : "no" ) << | ||
iendl << "the manifold dimension is " << descWrap.manifold().representationDim() | ||
<< decindent << iendl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the iendl
here (last line): the last CR is the responsibility of the function calling the print method (e.g. std::cout << my_object << std::endl
). Here you would sometimes add trailing whitespaces for no reason.
a451fe3
to
6d4858d
Compare
throw std::runtime_error (error->str()); | ||
} | ||
{ | ||
std::stringstream* error = new std::stringstream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for using the heap here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason. Thanks for reporting it. It will be fixed in the next PR.
Warning: Underlying manifolds library does not support Sparse-based manifolds. The manifold_jacobian function therefore only partially supports Sparse inputs.
Merging into master. |
Support of the sparse matrices in roboptim-core-manifold.
👍 |
The goal is to make it possible for roboptim-core-manifold users to define and use all the tools of the project in a sparse environment.
The support is still partial because the jacobian on the tangent space can't be filled right now on libmanifolds side. No more assumptions done, the call to Dispatcher::applyDiff will simply fail (throwing an std::string) in Sparse mode. Note that this function is obviously never called when the user do not need to compute the jacobian on the tangent space of the manifold.