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
Fix integrate method for SO2 #1652
Conversation
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.
@Toefinder Thanks for the fix. It is a bit weird to use the same input/ouput arguments at first glance.
For readability, could you remove the line commits which are related to whitespace?
Thanks in advance,
92e0c92
to
bbd3182
Compare
I thought so too, when first examining the code. But integrating in-place is used a lot as op.integrate(e_.template segment<LiegroupType::NQ>(iq_, op.nq()),
v_.segment<LiegroupType::NV>(iv_, op.nv()),
e_.template segment<LiegroupType::NQ>(iq_, op.nq())); |
I would say that because this part of the code is not more used with operator+= and some compilers may optimise some part of this code. |
When the same vector reference is given to the integrate method as both for input and for storing the output, the result is incorrectly calculated due to inplace changes. This problem is fixed in this commit.
bbd3182
to
e2c2ff4
Compare
@Toefinder I've just force-pushed to align the histories. |
61ab465
to
af973e9
Compare
Thanks @Toefinder, for your contributions. Greatly appreciated. |
When the same vector reference is given to the integrate method as both
for input and for storing the output, the result is incorrectly
calculated due to inplace changes. This problem is fixed in this commit.
Close #1651