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

unify interp_motion to match linear_axis/vel the other motions #273

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

geoynomous
Copy link
Contributor

@geoynomous geoynomous commented Mar 27, 2018

The Interpolation motion returns a linearVelocity() with is a vector, in contrast to other motions that split the value into linearVelocity() (a scalar) and linearAxis() which is a normalized vector. In this PR we're adapting the Interpolation motion to match the other motions.

Also added a first set of unit tests.

Question: Most motions have a test whether dt > 1 and if so they set dt = 1. Why is this test one-sided? That is, why not testing for dt < 0 and setting dt = 0 then?


This change is Reviewable

also added a first set of unit tests for interp_motion
@geoynomous
Copy link
Contributor Author

geoynomous commented Mar 28, 2018

hey - I see that some tests failed - basically Win32 and Debug version ... it's the equality test of transformations .... how you test for (near) equalities?

@SeanCurtis-TRI
Copy link
Contributor

Do you have access to a windows machine? In my limited history with FCL's CI, the times it failed on Windows I was able to reproduce locally (but only on a windows machine).

@SeanCurtis-TRI
Copy link
Contributor

Assuming it's only a rounding issue, you can use Eigen's isApprox method to apply a threshold.

Although, I am curious why its equal to the last bit in unix but not windows....

@SeanCurtis-TRI
Copy link
Contributor

Also, it might be worth stealing some code from Drake -- the gtest feedback on comparing matrices is a lot more helpful beyond a "nope...didn't match"

@geoynomous
Copy link
Contributor Author

No access to windows ... I'll check isApprox and Drake's code .. wonder if such essential code shouldn't be part of Eigen itself

@SeanCurtis-TRI
Copy link
Contributor

If I get a few cycles, I'll give it a stab on my windows VM. I'll let you know. But, certainly, the isApprox with an appropriate threshold is a good simple stab in the dark and then you can just wait to see what CI does.

@geoynomous
Copy link
Contributor Author

ouffff .. also isApprox does not give correct answers ... what's wrong on these installations?

@sherm1
Copy link
Member

sherm1 commented Mar 29, 2018

@geoynomous, the test failures appear to be attempts to do exact matches on floating point numbers, not isApprox(). For example, see the Travis log here (scroll down until after the tests execute to see the test output).

@sherm1
Copy link
Member

sherm1 commented Mar 29, 2018

For example:

/home/travis/build/flexible-collision-library/fcl/test/test_fcl_interp_motion.cpp:117: Failure
Value of: linear_axis == Vector3<S>(0.6, 0.8, 0.0)
  Actual: false
Expected: true

@sherm1
Copy link
Member

sherm1 commented Apr 2, 2018

CI looks much better. On Travis the only failures were the unresolved Mac problems that are independent of this PR. On Appveyor (Windows) there was a real test failure though:

22/22 Test #22: test_fcl_interp_motion ............***Failed    0.03 sec
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FCL_MATH
[ RUN      ] FCL_MATH.interp_motion_test_motion
C:\projects\fcl\test\test_fcl_interp_motion.cpp(73): error: Value of: itf.translation().isApprox(ctf.translation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(74): error: Value of: itf.rotation().isApprox(ctf.rotation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(78): error: Value of: itf.translation().isApprox(ctf.translation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(79): error: Value of: itf.rotation().isApprox(ctf.rotation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(83): error: Value of: itf.translation().isApprox(ctf.translation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(84): error: Value of: itf.rotation().isApprox(ctf.rotation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(94): error: Value of: itf.translation().isApprox(ctf.translation())
  Actual: false
Expected: true
C:\projects\fcl\test\test_fcl_interp_motion.cpp(95): error: Value of: itf.rotation().isApprox(ctf.rotation())
  Actual: false
Expected: true
[  FAILED  ] FCL_MATH.interp_motion_test_motion (7 ms)
[----------] 1 test from FCL_MATH (7 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (7 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FCL_MATH.interp_motion_test_motion
 1 FAILED TEST
95% tests passed,

@geoynomous
Copy link
Contributor Author

I've been looking at this again .... there must be something with the configuration of the test instances - as most test (in particular on Linux and newer compilers) work well .... so it's worth to investigate whether the arithmetic performed on the failing (mostly windows) platforms is correctly installed - single vs double precision .. similar to #291

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.

3 participants