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

Add define option DELAUNATOR_SINGLE_PRECISION to allow for the use of float instead of double. #18

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

Conversation

c0rp3n
Copy link

@c0rp3n c0rp3n commented Jul 8, 2020

Add support to switch to using single precision instead of double precision floating point values in delaunator.
This allows the user to define DELAUNATOR_SINGLE_PRECISION to toggle the usage of single precision floating point values.

Further had to include the header cassert in order for this to compile on MSVC 19.26.28806

Edit:
Added base support to the cmake file, should use a config file though for this.
3 Tests currently fail when using this due too the loss of precision

Added config support for using this, the same could be done for the single header.

c0rp3n added 4 commits July 8, 2020 15:33
precision floating point values in delaunator.
This allows the user to define DELAUNATOR_SINGLE_PRECISION to toggle the usage of single precision floating point values.

Further had to include the header `cassert` in order for this to compile on MSVC 19.26.28806
This is more of a hack to check run the tests with float's instead of double's, really need to added a config header to handle this instead of passing a public define to the target.
Currently 3 fail due to precision errors.
@abellgithub
Copy link
Owner

Does this work OK in practice? I'm a little concerned about the tests failing.

What's the motivation for this? Do you only have a processor that doesn't single-precision floating point?

@c0rp3n
Copy link
Author

c0rp3n commented Jul 10, 2020

So my main motivation is also allowing for the use of floats or doubles in my own library as generally dealing with ~160,000 and some of the code I intend to also use on gpu's which may have less double-fpu's than single-fpu's.

I havent seen any invalid triangulation as a result of using single precision, but my use case the points are guaranteed to be a decent distance apart.

The tests that fail are due to small rounding errors here is a copy from my machine.

E:\programming\delaunator-cpp\test\delaunator.test.cpp(29): error: Expected equality of these values:
  tri_area
    Which is: 1.1318623341473398e-16
  hull_area
    Which is: 1.1318615400539517e-16
[  FAILED  ] Delaunator.robusttiny (2 ms)
...
E:\programming\delaunator-cpp\test\delaunator.test.cpp(29): error: Expected equality of these values:
  tri_area
    Which is: 175963808
  hull_area
    Which is: 175963792
[  FAILED  ] Delaunator.robustbig (1 ms)
...
E:\programming\delaunator-cpp\test\delaunator.test.cpp(29): error: Expected equality of these values:
  tri_area
    Which is: 13.84178638458252
  hull_area
    Which is: 13.841787338256836
[  FAILED  ] Delaunator.robustmod2 (5 ms)

I also realise I didnt switch the tests to use EXPECT_FLOAT_EQ instead of EXPECT_DOUBLE_EQ so will look into whether this changes any results, so will add this to the test file too.

Edit: added what the tests where

@abellgithub
Copy link
Owner

abellgithub commented Jul 16, 2020

Sorry. I'll try to get to this shortly. I'm not convinced that it might not blow up with some cases, but I guess if a user wants to run that risk, that's on them.

@abellgithub
Copy link
Owner

Just changing the types doesn't make the tests pass. I'd have to take time to look at the issues and how best to deal with them. Some tests could be skipped when using single precision, I guess. If you want to look into this, that would be great, otherwise, you'll have to wait until I have more time to investigate.

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