flexible-collision-library / fcl

Flexible Collision Library

https://flexible-collision-library.github.io/

Other

1.41k stars 417 forks source link

Mac CI using single-precision libccd #291

Closed SeanCurtis-TRI closed 5 years ago

SeanCurtis-TRI commented 6 years ago

It appears that the mac ci is using single-precision libccd.

290 failed the new unit tests. Instead of getting solutions to a tolerance of 1e-14, it was getting results satisfied to within 1e-7. Furthermore, when evaluating the tests on a local mac with known double-precision libccd, these errors did not appear. But when forcing it to use single-precision libccd, the errors appeared.

scpeters commented 6 years ago

I think you are correct, though the homebrew formula for libccd is trying to use double precision:

https://github.com/Homebrew/homebrew-core/blob/master/Formula/libccd.rb#L18

I'm investigating

SeanCurtis-TRI commented 6 years ago

You have no idea how overjoyed that statement makes me.

scpeters commented 6 years ago

It looks like the libccd formula is using the latest tag v2.0, which is from 2014. In this version, you must pass -DCCD_DOUBLE=ON to use double precision. The latest version of the master branch uses -DENABLE_DOUBLE_PRECISION=ON and is in the documentation, so this was used in the homebrew formula ( https://github.com/Homebrew/homebrew-core/pull/22359#discussion_r161423077 ).

In the short term, we should update the libccd formula with the corrected cmake argument. We can also ping the libccd maintainer to see if they can make another release.

cc @jslee02

SeanCurtis-TRI commented 6 years ago

Being largely mac ignorant, I'm going to cast my vote modulo what actually works.

I like the flavor of not having to wait for libccd but I also like the idea of keeping the simplest of dependent relationships. So, I'd vote:

Let's do a local patch/fix/hack so that we get the right results with a big fat TODO contingent on a new release.
At the same time, let's ping libccd to get them to make a release.

SeanCurtis-TRI commented 5 years ago

This needs a refresh.

The homebrew formula now specifically refers to libccd v2.1.

However, between libccd v2.0 to v2.1, the CMake mechanism for specifying double precision changed from the macro CCD_ENABLE in 2.0 to ENABLE_DOUBLE_PRECISION in 2.1. However, the formula still uses the 2.0 spelling.

So, it seems we've reversed our old problem where, originally, we were pulling v2.0 but using the v2.1 spelling. Now we're using v 2.1 but using the v2.0 spelling.

I'm not a mac user, so I'd ask for one of the previous supporters of mac to help out on this one. If no one volunteers to do so, I'll step in. @scpeters @jslee02?

jslee02 commented 5 years ago

PR is submitted: https://github.com/Homebrew/homebrew-core/pull/37365

mamoll commented 5 years ago

It doesn't make a difference in the 2.1 release. Both ways of specifying precision are supported: https://github.com/danfis/libccd/blob/v2.1/src/CMakeLists.txt#L1-L17 Still, not a bad idea to switch to the new preferred way before the old way is deprecated.

SeanCurtis-TRI commented 5 years ago

Given that with that observation and all of the discussion in the homebrew pr, we can safely close this issue.

(Feel free to re-open this if you feel I've been overly optimistic.)