Inclusion of MPFR for Turaev-Viro computation

[orouille]

Lack some commenting and the testing part of the MFloat is very light, wanted the confirmation that the "shape of the code was accepted" before going into that

[baburton]

Hi Owen - thanks for this.

In general the shape of the code looks good to me.

My biggest concern is that the MFloat type is not thread-safe, in that the precision is a static variable. It's a reasonable thought that people might be computing T-V invariants in parallel, and actually it doesn't seem too unreasonable that in a large and complex setting people might be wanting to do parallel computations of different things using different precisions. (AFAICT, GNU MPFR supports this by having a precision that is "local" to each number, but I never use MPFR so I could be wrong.)

My suggestion here would be either to use the precision as a template argument, or - if you want to support a choice of precision at runtime - to include it as an argument in the constructor and then "inherit" it from arguments during calculations, much like MPFR itself seems to (as I look at the MPFR API).

Regarding the T-V code: it should also behave well in settings where there is no MPFR. One option here would be to throw an exception if regina cannot give the precision you are asking for (e.g., because MPFR is not available); another option would be to pass the floating point type as a template parameter to tuarevViroApprox() (instead of passing a precision argument), which means you are also giving people access to long double if they want it (though this will restrict your ability to "choose" a precision at runtime).

Finally: I don't usually live in the floating point world, so I'm not sure what the C++ specification says about how double is implemented; however, if 64-bit is not mandated then I'd suggest replacing 64 with an appropriate compile-time constant (e.g., sizeof(double), or something from std::numeric_limits) so that the code does the right thing on all platforms. If you do want to try things out on more exotic platforms, you can put some tests in the test suite that probe different size/precision barriers (like I've done with the integer classes) - the Debian auto builders will be running these tests on a ton of different platforms so you will get to see if things go wrong. (And if you need it, I have access to Debian build machines so if you're not sure if it will work on architecture X then let me know and I can log on to such a machine and try it out.)

Best regards - Ben.

[orouille]

Hi Ben, Thank you for the detailed answer.

I switched to local precision for the mfloats: each number has its precision, to do that I needed to specify the precision at each initialization. See for instance initLongInt replacing the "TVType valColour(init.halfField ? init.r : 2 * init.r);".
Thus the precision can be modified at runtime, I don't think this will be used but I encountered a problem I could not solve trying to use a templated type.

I don't master cmake so I did not put the right name in the ifdef, I used __REGINA_MFLOAT. Just replace this name with the right one and it should be fine.

Finally: the compiler is happy with sizeof(double) as default value. I don't have many different machines, beside mine and the sever. Once the tests are written, I might need a bit of help, depending on your expectations.

Are you satisfied with these modifications are there other problems?

Yours,
Owen

---

PS: To add to the "no MPFR problem", a similar solution to what's done with the choice of algorithms can be used. With the current implementation, asking for multiprecision wihtout MPFR will not raise an exception, it will simply use the doubles. Rising something can easily be added.