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

Solve the Math.PI syntax displays wrong value #450

Merged
merged 7 commits into from Jan 10, 2020

Conversation

@maopeixia
Copy link

maopeixia commented Dec 27, 2019

No description provided.

@joelmartinez joelmartinez self-assigned this Jan 2, 2020
Copy link
Member

joelmartinez left a comment

Very simple change 👍 However, I'd like to see a unit test to verify that it will result in the output that #412 is looking for ... namely, that it will result in the correct value for Math.PI. We should see whether the feedback/recommendation in this comment is necessary to get the result we're expecting, or whether it will work at all given that mdoc isn't running on .net core 3.0

string value = ((IFormattable)val).ToString(null, CultureInfo.InvariantCulture);
string value = "";
if (field.FieldType.FullName == "System.Double")
{ value = ((IFormattable)val).ToString("R", CultureInfo.InvariantCulture); }

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jan 3, 2020

I don't think using R is necessarily appropriate.

On Full Framework, there is no guarantee that R provides a string that will actually roundtrip (due to issues in the floating-point parser that can't be fixed due to compat reasons, etc; for .NET Core 3.0 and later, R and G will always roundtrip).

The appropriate thing is to likely use G17 which will produce enough digits to roundtrip. 17 is the maximum precision supported by full framework (.NET core supports up to 99). G17 will always produce a roundtrippable string on .NET Core and will in almost every scenario produce a roundtrippable string on full framework.

G9 is the precision that should be used for System.Single

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jan 3, 2020

I don't know about the guarantees of Mono; but I believe it is sharing the parsing/formatting code with .NET Core; so it likely depends on the version of Mono being used.

Copy link
Member

joelmartinez left a comment

I agree with @tannergooding ... changing the format strings for double to G17 and single to g9 is the ideal solution. We're close here :)

@tannergooding

This comment has been minimized.

Copy link

tannergooding commented Jan 3, 2020

Separately from changing the value string displayed in the documentation; we probably want a short snippet for core values (like Math.PI) explaining why the value shown to 17 digits isn't the "expected" value.

For example, the actual values are approx:

MathF.PI:  3.14159274...
Math.PI:   3.1415926535897931...
Actual PI: 3.1415926535897932384626433...

Some people may expect MathF.PI to end in 65 rather than 74 and for Math.PI to end in 32 rather than 31; but float/double are merely the nearest representable value.

@joelmartinez

This comment has been minimized.

Copy link
Member

joelmartinez commented Jan 6, 2020

@tannergooding so @maopeixia has made this change, however one of the existing tests is having a regression. As you can see here, there was an existing test that used an approximation of PI (how appropriate :P ) as a test value:

protected internal const double PI = 3.14159;

Unfortunately, as you can see here in the diff ... using G17 for double results in the following changed values:

-      <MemberSignature Language="C#" Value="protected internal const double PI = 3.14159;" />
-      <MemberSignature Language="ILAsm" Value=".field familyorassembly static literal float64 PI = (3.14159)" />
+      <MemberSignature Language="C#" Value="protected internal const double PI = 3.1415899999999999;" />
+      <MemberSignature Language="ILAsm" Value=".field familyorassembly static literal float64 PI = (3.1415899999999999)" />

Not exactly a more accurate result :| I'd like to propose that we go back to using R for both double and single, and let newer releases of mono/framework catch up to that new .net core 3 implementation over time. Are you ok with that?

@maopeixia

This comment has been minimized.

Copy link
Author

maopeixia commented Jan 7, 2020

I've went back to using "R" format.

@tannergooding

This comment has been minimized.

Copy link

tannergooding commented Jan 7, 2020

Not exactly a more accurate result

Actually, it's significantly more accurate. It shows that 3.14159 is not actually 3.14159 and is instead 3.14158999999999988261834005243144929409027099609375 (due to the original value not being exactly representable).

You ultimately need to decide between having accurate roundtrippable numbers that may be overly verbose or returning values that are "pretty" but which may have off by one (or more) errors when parsed.

On .NET Framework and .NET Core prior to 3.0, using G is equivalent to G15. Using R is equivalent to attempting G15, parsing to see if the value roundtrips and using G17 if that fails. However, due to bugs in the parser, this isn't guaranteed to produce a roundtrippable result and may incorrectly truncate certain values.

On .NET Core 3.0 and later, G is equivalent to R and both always produce the shortest roundtrippable string.

A (non-exhaustive) selection of the issues that were resolved due to the above issues and which go into more detail are here:
dotnet/coreclr#1316, dotnet/coreclr#3313, dotnet/coreclr#13106, dotnet/coreclr#13615, dotnet/coreclr#17467, dotnet/coreclr#19802, dotnet/corefx#31847, etc

Copy link
Member

joelmartinez left a comment

ok, "accurate" was the wrong word to use :P However I think I'm going to stick to to using "R" here, because mdoc runs in our docs CI on a windows agent, and thus with the .net framework ... and using G17 for the original value that started this whole thing (Math.Pi), gives us the exact same value as using R so we're not really gaining anything from using G17 for doubles in this case (or the approximated test case mentioned in my previous comment):

image

Once .NET 5 comes out ... mdoc will be migrating to that, at which point we'll get the behavior that we see in .NET Core 3.

@joelmartinez joelmartinez changed the base branch from marj-metadata to develop Jan 10, 2020
@joelmartinez

This comment has been minimized.

Copy link
Member

joelmartinez commented Jan 10, 2020

Closing and re-opening to kick off CI

@joelmartinez joelmartinez reopened this Jan 10, 2020
@joelmartinez joelmartinez merged commit 9f26553 into mono:develop Jan 10, 2020
1 check passed
1 check passed
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.