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 float/double mismatches in source #150

Closed
hollasch opened this issue Aug 17, 2019 · 14 comments
Closed

Fix float/double mismatches in source #150

hollasch opened this issue Aug 17, 2019 · 14 comments

Comments

@hollasch
Copy link
Collaborator

Lots of places where we're using double constants for use in float calculations, mixing up our widths when passing parameters, and such. We need to ensure that our code compiles without any warnings about mixing precision.

Ideally, it would be good to determine what the impact of float vs. double. Personally, I prefer double precision, as it's the native precision for math, and C++ constants, and is frequently faster on platforms that perform intrinsic computation in double-precision (and thus induce a performance hit when transforming from and to single-precision.

@trevordblack
Copy link
Collaborator

trevordblack commented Aug 17, 2019 via email

@trevordblack trevordblack self-assigned this Aug 17, 2019
@trevordblack
Copy link
Collaborator

I don't believe that my comment above is warranted. We should pick a precision type and stick with it.

I went ahead and created a benchmark.
On my own
Intel® Core™ i7-9750H CPU @ 2.60GHz × 12
running in Ubuntu 18.04.3 LTS
Compiled with Clang6.0.0 ubuntu
I found:
double: 442 seconds
float: 456 seconds
The difference is noise.

If more data is warranted,
$ git pull
$ git checkout origin/tdb/real_type_exp
$ cd src
$ mkdir build
$ cd build
$ cmake ..
$ make
$ time ./Real_Type_Double > double.ppm && time ./Real_Type_Float > float.ppm

To Be Clear
This is not a production branch.
Once a decision of floats vs doubles is made, I can go through and replace Reals with the selection.

@hollasch
Copy link
Collaborator Author

I also had the idea of using real = double or using real = float. However, your benchmark bears out my experience, that float is not a performance optimization, and indeed, working entirely in double values often yields a (tiny) performance benefit. Old habits die hard, though.

I say we convert the entire code base to use simple double-precision floats.

Anybody else care to comment?

@ronaldfw
Copy link
Contributor

If double is not slower than float (single precision) here, I'd vote for just using double everywhere. With float you could get performance benefits when implementing certain parts with SIMD (SSE, AVX) but I don't think we should go there (and I don't know if there are even places where that would work well in any of the code here).

@hollasch
Copy link
Collaborator Author

I think we're starting to lean toward double at this point. The only reason to use single-precision is that GPUs typically work with floats, but we're not in that domain. Converting to float is fine for storage purposes, but we're not doing that either.

@trevordblack
Copy link
Collaborator

From all of this discussion, what I've arrived at is that the only significant difference between floats and doubles for this domain is that floats are much more likely to have shadow acne.

This is arguably a benefit because it would much more clearly illucidate the problem.

Other than that, I'm not seeing any real argument that isn't aesthetic. My vote is for doubles.

@hollasch
Copy link
Collaborator Author

I suspect that adopting double precision may not change the frequency of surface acne. Does it matter if your intersection point is 1e-10 inside the surface versus 1e-100 inside? Should be a simple test to verify, but agree with Trevor that it shouldn't really affect the decision.

Sounds like we're agreed on using double.

vchizhov referenced this issue in vchizhov/InOneWeekend-1 Aug 23, 2019
Compiles with no warnings on MSVC 2019 with Warning Level: /W4.

- provided CMakeLists.txt (tested only on Windows 10 with MSVC 2019 though, create the project in /build so that it would be ignored by the gitignore)
- changed M_PI with a constexpr pi<T>() function
- substituted random_double() and drand48() with my own random() function using the Mersenne twister from the standard library
- use fstream rather than cout, so that redirecting would not be required
- fixed double/float mismatch

Removed random.h since it is redundant - I substituted random_double() with my own random() implementation using the Mersenne twister from the standard library. The random_double() in random.h used rand() % (RAND_MAX + 1.0); which is suboptimal (fails multiple statistical tests).
@hollasch hollasch transferred this issue from RayTracing/InOneWeekend Aug 30, 2019
@hollasch hollasch added this to the post-v2 milestone Aug 30, 2019
@vchizhov
Copy link

vchizhov commented Sep 1, 2019

I am for using float rather than double. The main argument towards using double that I managed to gather from here was self intersection.
However self-intersection occurs even with double precision. And thus should be handled in a different manner, see #137.
On the other hand using float allows for a straightforward extension of the code through SIMD, as well as a minor scalar code speed up.

@hollasch
Copy link
Collaborator Author

hollasch commented Sep 6, 2019

No, see my comment just above yours. Self intersection is not one of the arguments. Instead,

  • double is frequently faster, since it can avoid conversion overhead. This is not an argument in favor, however, since ultimate performance is not a goal of this book.
  • Most math libraries are double-precision in arguments and return value.
  • Floating point literals are double-precision.

@vchizhov
Copy link

vchizhov commented Sep 7, 2019

As far as the comment above mine goes, double vs float affects self intersection. It is not so much about whether it's 0.1 or 0.01 on the inside, but rather whether it ends up on the inside in the first place due to round-off error.

What conversion are we talking about? And why would double computations be faster? This is untrue for many machines, compilers, and compilation options. And even less true for SIMD.

The math libraries that I know of usually provide both single and double precision versions of the functions, notably so does stl.

1.0f is a floating-point literal. The f is a type specifier. So I am unsure whether the argument is that f is undesirable?

@hollasch
Copy link
Collaborator Author

hollasch commented Sep 7, 2019

See timing results above. This is not conclusive, but generally true that float can induce conversion costs to and from double (or higher) internal precision for computation. It doesn't matter, though, since the difference is tiny and irrelevant for our purposes. SIMD is a non-issue — it is outside of the scope of this book, and as I wrote above, high performance is not a goal.

The code should be most broadly useful, and I know of no platforms that provide only single-precision math libraries, while I know some that provide only double.

Regarding constants, yes, I was talking about the C-centric annoyance of having to suffix all numbers with f, both internal and imported. double can accept both single and double-precision values, float will give you warnings.

Readers are strongly encouraged to write the raytracing code themselves. The code included in the book is for illustration, and meant to be broadly understood by programmers regardless of their preferred languages. Using double is a tiny improvement in that regard.

@vchizhov
Copy link

vchizhov commented Sep 7, 2019

I thought that the benchmark conclusion was that the "speed up" was due to noise. One also doesn't need to convert between float and double if they keep computations in float.

Using double and the code being broadly useful clashes with SIMD and GPU ports, while allowing for no warnings double only libraries integration. Ideally a typedef should be used if the code is to be broadly useful, but that's beyond the point.

The f is a valid argument - I've had to change 1.0 to 1.0f when porting from glsl.

I still think it's wasteful using double, where float is valid, but this is mostly optimization speaking, and I can see where you're coming from with double.

@hollasch
Copy link
Collaborator Author

hollasch commented Sep 7, 2019

Conversion happens in the FPU. Scalar floating point is computed at 64 or 80-bit precision on most architectures. Regardless

  • We are not basing this decision on performance
  • SIMD and GPU ports are not a goal
  • Readers are encouraged to write their own raytracing code

hollasch added a commit that referenced this issue Oct 19, 2019
hollasch added a commit that referenced this issue Oct 20, 2019
@hollasch
Copy link
Collaborator Author

Fixed in #222 (development branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants