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

Mac CI using single-precision libccd #291

Closed
SeanCurtis-TRI opened this issue May 23, 2018 · 8 comments
Closed

Mac CI using single-precision libccd #291

SeanCurtis-TRI opened this issue May 23, 2018 · 8 comments

Comments

@SeanCurtis-TRI
Copy link
Contributor

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
Copy link
Contributor

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

I'm investigating

@SeanCurtis-TRI
Copy link
Contributor Author

You have no idea how overjoyed that statement makes me.

@scpeters
Copy link
Contributor

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 ( Homebrew/homebrew-core#22359 (comment) ).

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
Copy link
Contributor Author

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:

  1. 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.
  2. At the same time, let's ping libccd to get them to make a release.

@SeanCurtis-TRI
Copy link
Contributor Author

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
Copy link
Member

jslee02 commented Feb 26, 2019

PR is submitted: Homebrew/homebrew-core#37365

@mamoll
Copy link
Member

mamoll commented Feb 26, 2019

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
Copy link
Contributor Author

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.)

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

No branches or pull requests

4 participants