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.

Sign up for GitHub

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

Jump to bottom

Use scientific to fix floating point rounding problems #58

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use scientific to fix floating point rounding problems #58

wants to merge 3 commits into from

Conversation

mightybyte
Copy link

Here's a proof of concept. Do you have a preference about how to resolve the LANGUAGE Safe bit? I was unsure about that.

@andreasabel
Copy link
Member

Testsuite fails, missing instances From/ToYaml Double:

[1 of 1] Compiling Main             ( tests/Tests.hs, /__w/HsYAML/HsYAML/dist-newstyle/build/x86_64-linux/ghc-9.2.1/HsYAML-0.2.1.0/t/tests/build/tests/tests-tmp/Main.o )

tests/Tests.hs:26:[32](https://github.com/haskell-hvr/HsYAML/pull/58/checks#step:19:32): error:
    • No instance for (ToYAML Double) arising from a use of ‘outputStr’
    • In the second argument of ‘(==)’, namely ‘outputStr d’
      In the expression: ".nan" == outputStr d

tests/Tests.hs:60:32: error:
    • No instance for (FromYAML Double)
        arising from a use of ‘roundTrip’
    • In the second argument of ‘($)’, namely
        ‘roundTrip approxEq (1 :: Double)’

@sjakobi sjakobi linked an issue May 11, 2022 that may be closed by this pull request
Decimal numbers have rounding error #57
Open
@mightybyte mightybyte marked this pull request as ready for review October 28, 2022 06:32
sjakobi
sjakobi reviewed Oct 28, 2022
View reviewed changes
src/Data/YAML.hs
Comment on lines -476 to 477
instance FromYAML Double where
instance FromYAML Scientific where
parseYAML = withFloat "!!float" pure
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the rationale for removing FromYAML and ToYAML Double?

Copy link
Author

Choose a reason for hiding this comment

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

I guess there's an argument for going either way. My thought was that it's mainly there for the vales actually supported as scalar primitives, and since this PR changes the scalar numeric primitive from Double to Scientific, that seemed to be the obvious route to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, we also provide instances for types like Int8 and tuples, so I think we ought to keep the Double instances.

sjakobi
sjakobi reviewed Nov 2, 2022
View reviewed changes
cabal.project
@@ -1,4 +1,4 @@
packages: .

package HsYAML
flags: +exe
flags: +exe
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you undo this change?

src/Data/YAML.hs
Comment on lines -476 to 477
instance FromYAML Double where
instance FromYAML Scientific where
parseYAML = withFloat "!!float" pure
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, we also provide instances for types like Int8 and tuples, so I think we ought to keep the Double instances.

src/Data/YAML/Schema/Internal.hs
Comment on lines +455 to +456
encodeDouble :: Scientific -> T.Text
encodeDouble d = T.pack . show $ d
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function should be named encodeFloat for consistency with encodeInt etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, is there a more efficient way to perform this encoding?

@sjakobi
Copy link
Collaborator

sjakobi commented Nov 2, 2022

Do you have a preference about how to resolve the LANGUAGE Safe bit?

I don't really have any experience with -XSafe. I'm fine with removing it for now and waiting until someone comes along who cares. :)

@andreasabel
Copy link
Member

What is the error if LANGUAGE Safe is left there?

@ulysses4ever ulysses4ever mentioned this pull request May 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjakobi sjakobi sjakobi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decimal numbers have rounding error
3 participants
@mightybyte @andreasabel @sjakobi