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 some Json tests #5037 -followup #5070

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Conversation

OceanOak
Copy link
Collaborator

No changelog

#5037 follow-up

  • The float formatting precision was increased from %.12 to %.16 to match other programming languages (F#, python, and Javascript)

  • The added tests that use [number]e+[pow] were removed.

@@ -84,7 +84,7 @@ let rec serialize
else if System.Double.IsPositiveInfinity f then
w.WriteStringValue "Infinity"
else
let result = sprintf "%.12g" f
let result = sprintf "%.16g" f
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the float formatting precision to 16 makes it consistent with other languages. Is that sufficient, or do we want to move away from sprintf entirely?

@OceanOak
Copy link
Collaborator Author

From my reading, most languages seem to not explicitly set float formatting precision; the limitations typically are inherited from the IEEE 754 double precision standard.
Others use the the round-trip precision format "R" (link)
Some workarounds include using decimals instead of floats. Or allowing users to explicitly define precision through configuration arguments
example:

If use_decimal is true (default: True) then decimal.Decimal will be natively serialized to JSON with full precision.

some useful links and discussions:

I'm uncertain about our next steps. We can adhere to the IEEE 754 standard, which allows for up to 16 significant digits (link). This aligns with the consistent results we obtained while testing other languages
or maybe find a way to not explicitly use sprintf "%.16g"
Thoughts?

@OceanOak OceanOak marked this pull request as ready for review September 12, 2023 10:37
@pbiggar pbiggar merged commit c81ed83 into darklang:main Sep 14, 2023
6 checks passed
@OceanOak
Copy link
Collaborator Author

Paul's notes:

Here are some unorganized thoughts:

  • Float.toString should be handled too? (Float.toString -> Float.parse roundtrip)?
  • ChatGPT told me we might need 17 significant digits. It gave the example 0.0000000000000002220446049250313080847263336181640625 as something where it's more accurate with 17 digits than 16
  • I tried that example with sprintf "%.16g" and "%.17g" and both gave me something like 2.220446049250313e-16 - is that the right answer? I don't know
  • I also tried f.ToString("R") for roundtripping. Dunno what to do there.

More organized thoughts:

  • sprintf is definitely not right
  • we should come up with some test cases first - things that test the precision and also the UX (does the json need to be pretty as well as right? yes)
  • that seems like work, so maybe settle on just doing sprintf "%.16g" or "%.17g" in this PR, and noting the rest for work later

I've tried using sprintf "%.17g" but I didn't like the result we got from some tests.
Here is an example :

Builtin.Json.serialize<PACKAGE.OpenAI.Completion.Request> (
  PACKAGE.OpenAI.Completion.Request
    { model = "davinci"
      prompt = "test"
      max_tokens = 5
      temperature = 0.7 }
) = PACKAGE.Darklang.Stdlib.Result.Result.Ok
  "{\"max_tokens\":5,\"model\":\"davinci\",\"prompt\":\"test\",\"temperature\":0.69999999999999996}"

We've decided to use sprintf "%.16g" for now and plan to remove it later.

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.

None yet

2 participants