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 failed unit tests on 32-bit machine #368

Merged
merged 20 commits into from
Apr 11, 2018

Conversation

dqyi11
Copy link
Contributor

@dqyi11 dqyi11 commented Apr 7, 2018

This PR fixes the failed unit tests on 32-bit machine,
which include (1) precision difference #220 ; and (2) Eigen alignment #365 .

In fixing the Eigen alignment issues, the updates include
(1) add EIGEN_MAKE_ALIGNED_OPERATOR_NEW macros in structures having fixed-size Eigen members;
https://eigen.tuxfamily.org/dox/group__TopicStructHavingEigenMembers.html
(2) add aligned allocator when using STL containers with fixed-size Eigen members.;
https://eigen.tuxfamily.org/dox/group__TopicStlContainers.html
(3) Replacing std::make_shared with dart::common::make_aligned_shared for aligned structures.

  • Document new methods and classes (N/A)

  • Format code with make format

  • Set version target by selecting a milestone on the right side

  • Summarize this change in CHANGELOG.md

  • Add unit test(s) for this change (N/A)

@dqyi11 dqyi11 added this to the Aikido 0.3.0 milestone Apr 7, 2018
@dqyi11 dqyi11 requested a review from jslee02 April 7, 2018 00:47
Copy link
Member

@jslee02 jslee02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. I have one suggestion though.

@@ -79,6 +79,9 @@ class SE2BoxConstraint : public constraint::Projectable,

// DOF of the joint
std::size_t mDimension;

public:
EIGEN_MAKE_ALIGNED_OPERATOR_NEW
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this for this class because none of the member variables is the vectorizable Eigen object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses Eigen::Vector3d in defining mLowerLimits and mUpperLimits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fixed-size of odd dimension is not a vectorizable Eigen object. 😈 If you look at the link, all the vectors are of even dimensions. Affine3d (3x4 or 4x4; I don't remember what was it among them though.) is actually even dimension.

@codecov
Copy link

codecov bot commented Apr 8, 2018

Codecov Report

Merging #368 into master will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
- Coverage   83.41%   83.37%   -0.04%     
==========================================
  Files         197      197              
  Lines        5614     5619       +5     
==========================================
+ Hits         4683     4685       +2     
- Misses        931      934       +3
Impacted Files Coverage Δ
...r/vectorfield/MoveEndEffectorOffsetVectorField.hpp 100% <ø> (ø) ⬆️
...ner/vectorfield/MoveEndEffectorPoseVectorField.hpp 100% <ø> (ø) ⬆️
include/aikido/distance/SE2Weighted.hpp 50% <0%> (-50%) ⬇️
...de/aikido/constraint/uniform/RnConstantSampler.hpp 50% <0%> (-50%) ⬇️
...nstraint/uniform/detail/RnConstantSampler-impl.hpp 100% <100%> (ø) ⬆️
src/common/StepSequence.cpp 93.54% <100%> (ø) ⬆️
src/planner/vectorfield/VectorFieldPlanner.cpp 75% <100%> (ø) ⬆️
...lude/aikido/constraint/uniform/RnBoxConstraint.hpp 100% <100%> (ø) ⬆️
src/planner/vectorfield/VectorFieldUtil.cpp 93.54% <50%> (-1.54%) ⬇️
src/trajectory/Spline.cpp 93.51% <0%> (-0.93%) ⬇️
... and 1 more

jslee02
jslee02 previously approved these changes Apr 10, 2018
@jslee02 jslee02 merged commit d84453a into master Apr 11, 2018
@jslee02 jslee02 deleted the bugfix/dqyi/stepSequenceOn32BitMachine branch April 11, 2018 16:04
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