Expression: use a precision that ensures no floating point issue

[0penBrain]

https://forum.freecad.org/viewtopic.php?t=77287

I should say I didn't really understand current code.
std::numeric_limits<double>::digits10 is exactly the maximum precision to ensure no floating point rounding issue.
So setting it to + 1 is just making a step to the world of problems. 😄

[freecadci]

for feature branch PR_9163. Pipeline 826746123 was triggered at 98b0d75. All CI branches and pipelines.

[wwmayer]

I don't think it's that easy. Have a look at https://en.cppreference.com/w/cpp/types/numeric_limits/max_digits10
when writing a number.

with this test function

#include <limits>
#include <iomanip>
#include <iostream>
#include <sstream>
#include <cmath>

int main()
{
    {
        std::ostringstream str;
        str << std::setprecision(std::numeric_limits<double>::digits10 + 1);
        str << 8.2 << '\n';
        double next = std::nextafter(8.2, 9.0);
        str << next << '\n';
        str << std::fixed << std::numeric_limits<double>::epsilon() << '\n';
        std::cout << "\nUse " << std::numeric_limits<double>::digits10 + 1 << " digits\n";
        std::cout << str.str();
    }
    {
        std::ostringstream str;
        str << std::setprecision(std::numeric_limits<double>::digits10);
        str << 8.2 << '\n';
        double next = std::nextafter(8.2, 9.0);
        str << next << '\n';
        str << std::fixed << std::numeric_limits<double>::epsilon() << '\n';
        std::cout << "\nUse " << std::numeric_limits<double>::digits10 << " digits\n";
        std::cout << str.str();
    }
}

the output is:

Use 16 digits
8.199999999999999
8.200000000000001
0.0000000000000002

Use 15 digits
8.2
8.2
0.000000000000000

[0penBrain]

I don't think it's that easy.

Hmm. Sorry I don't see. For me all values on top are wrong, and all on bottom are correct. So it seems to confirm the fix. What did I miss?

[wwmayer]

What did I miss?

The value of double next = std::nextafter(8.2, 9.0); is guaranteed to be different to 8.2. When writing with digits10 the text output is the same, i.e. when reading it in we have a loss of accuracy.
Also the value of std::numeric_limits<double>::epsilon() is guaranteed to be higher than the written 0.0

The documentation of max_digits10 says:

Unlike most mathematical operations, the conversion of a floating-point value to text and back is exact as long as at least max_digits10 were used (9 for float, 17 for double): it is guaranteed to produce the same floating-point value, even though the intermediate text representation is not exact. It may take over a hundred decimal digits to represent the precise value of a float in decimal notation.

So, this means when serializing a double as string then by using max_digits10 (which is even higher than digits10 + 1) it's guaranteed that the double converted from the string is equal. With digits10 we lose precision.

Look at this test code now:

#include <limits>
#include <iomanip>
#include <iostream>
#include <sstream>
#include <cmath>

int main()
{
    {
        double value1 = std::asin(1);
        std::ostringstream str;
        str << std::setprecision(std::numeric_limits<double>::max_digits10);
        str << value1;
        double value2 = std::stod(str.str());
        std::cout << "double -> text -> double: " << (value1 == value2 ? "equal\n" : "not equal\n");
    }
    {
        double value1 = std::asin(1);
        std::ostringstream str;
        str << std::setprecision(std::numeric_limits<double>::digits10);
        str << value1;
        double value2 = std::stod(str.str());
        std::cout << "double -> text -> double: " << (value1 == value2 ? "equal\n" : "not equal\n");
    }
}

The output is:

double -> text -> double: equal
double -> text -> double: not equal

So, the question is whether the change could cause possible regressions elsewhere.

[0penBrain]

Well, all that you wrote is perfectly correct and I agree with it. But IMO this is looking at the problem from the wrong side. 😉

The value of double next = std::nextafter(8.2, 9.0); is guaranteed to be different to 8.2. When writing with digits10 the text output is the same, i.e. when reading it in we have a loss of accuracy.

Which is excellent. We have an inner precision better than the one we display. This is precisely what we want.
Oppositely, your previous posts proved that using max_digits10 +1 can't guarantee that all inputs aren't actually possible as 8.2 and its nextafter are separated by more than 1 less-significant digit.

Also the value of std::numeric_limits<double>::epsilon() is guaranteed to be higher than the written 0.0

Again, I have no doubt that it internally is. And having displayed as 0.0 is excellent as this also proves that our display precision is compatible with internal precision.

The documentation of max_digits10 says:

Unlike most mathematical operations, the conversion of a floating-point value to text and back is exact as long as at least max_digits10 were used (9 for float, 17 for double): it is guaranteed to produce the same floating-point value, even though the intermediate text representation is not exact. It may take over a hundred decimal digits to represent the precise value of a float in decimal notation.

True. But basically we do the opposite : string -> value -> string. And in this case this documentation doesn't apply. It even is the "opposite", we have issues using a more than max_digits10 value to string conversion. Which is normal.

So, this means when serializing a double as string then by using max_digits10 (which is even higher than digits10 + 1) it's guaranteed that the double converted from the string is equal. With digits10 we lose precision.

But we need to lose precision in our case. 😉 This is a tradeoff to ensure we correctly display the user input.

So, the question is whether the change could cause possible regressions elsewhere.

I quickly checked and toString() calls seem to be only used for presentation so it shouldn't create any regression.
And anyway if it would, that would be an error made somewhere else in the code to use this IMO.
And well, admittedly it solves the bug seen on the forum. 😉

[wwmayer]

I have looked at the history of the file and at the very beginning there was no precision set. Later digits10 + 1 was set, then digits10 + 2 that caused some problems. The change was reverted until it was again set to digts10 + 2. This of course, caused again some problems so that it was reverted.

However, I never found an explanation why digits10 +1 or +2 was used.

True. But basically we do the opposite : string -> value -> string.

Not only. When saving it to a file then we have value -> string -> value. Doing some tests I couldn't find a case where the restored value has caused problems.

I quickly checked and toString() calls seem to be only used for presentation so it shouldn't create any regression.

Then you missed the saving of a project. The call stack is:

• NumberExpression::_toString
• Expression::toString
• Expression::toString (overloaded version)
• Cell::getStringContent
• Cell::save
• Cell::save (overloaded version)
• PropertySheet::Save
• ...

But as said I couldn't find a problem I think it's fine to merge it.

[0penBrain]

Not only. When saving it to a file then we have value -> string -> value.

Yes, but this is actually part of a longer chain : string (user input) -> value (expression) -> string (document) -> value (expression) -> string (display). 😉

Then you missed the saving of a project.

Yes I did. 😄

[wwmayer]

Yes, but this is actually part of a longer chain : string (user input) -> value (expression) -> string (document) -> value (expression) -> string (display).

Not necessarily. I have created a test project with a spreadsheet. After the first saving I just opened the file and directly saved it again without having opened the spreadsheet.