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

fix missing DOF_ACCELERATION to dynamic variable ordering #587

Merged
merged 2 commits into from
Nov 13, 2019

Conversation

lrapetti
Copy link
Member

@lrapetti lrapetti commented Oct 3, 2019

The DOF_ACCELERATION is not add to the dynamicVariablesOrdering

@lrapetti
Copy link
Member Author

lrapetti commented Oct 3, 2019

CC @prashanthr05 @Yeshasvitvs

@traversaro
Copy link
Member

traversaro commented Oct 3, 2019

Thanks @lrapetti , can you also update the changelog at https://github.com/robotology/idyntree/blob/devel/doc/releases/v0_12.md ?

Furthermore, following Beyonce's Law:

"Cause if you like this code, then you shoulda put a test on it"

it would be nice to make sure that this code is tested, by generalizing the following test: https://github.com/robotology/idyntree/blob/master/src/estimation/tests/BerdyHelperUnitTest.cpp#L158 to the floating base case.

@lrapetti
Copy link
Member Author

lrapetti commented Oct 3, 2019

Thanks @lrapetti , can you also update the changelog at https://github.com/robotology/idyntree/blob/devel/doc/releases/v0_12.md ?

done db0eefb

@traversaro
Copy link
Member

To clarify, fixing the test is not strictly required to get this PR merged.

@lrapetti
Copy link
Member Author

lrapetti commented Oct 3, 2019

it would be nice to make sure that this code is tested, by generalizing the following test: https://github.com/robotology/idyntree/blob/master/src/estimation/tests/BerdyHelperUnitTest.cpp#L158 to the floating base case.

But is it also the test for fixed base currently commented out??https://github.com/robotology/idyntree/blob/master/src/estimation/tests/BerdyHelperUnitTest.cpp#L337-L341

@traversaro
Copy link
Member

But is it also the test for fixed base currently commented out??https://github.com/robotology/idyntree/blob/master/src/estimation/tests/BerdyHelperUnitTest.cpp#L337-L341

Good catch, I probably wrongly commented it out in and that slipped in in the review (probably due to the size, see #356 and https://twitter.com/iamdevloper/status/397664295875805184) . Actually I opened an issue on this back in #445 , this MAPest issue also seems related: claudia-lat/MAPest#28 .

@lrapetti
Copy link
Member Author

lrapetti commented Oct 3, 2019

After closing this PR I will try to fix the tests for the original berdy, and eventually add those for the floating base

@traversaro
Copy link
Member

@lrapetti I guess we can merge this PR even with no additional tests?

@lrapetti
Copy link
Member Author

@lrapetti I guess we can merge this PR even with no additional tests?

yess

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.

2 participants