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

Use scientific to fix floating point rounding problems #58

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions HsYAML.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ library
, text >= 1.2.3 && < 2.1
, mtl >= 2.2.1 && < 2.4
, parsec >= 3.1.13.0 && < 3.2
, scientific >= 0.3 && < 0.4
, transformers >= 0.4 && < 0.7

-- for GHC.Generics
Expand Down Expand Up @@ -154,5 +155,6 @@ test-suite tests
, mtl
-- non-inherited
, QuickCheck == 2.14.*
, scientific
, tasty == 1.4.*
, tasty-quickcheck == 0.10.*
2 changes: 1 addition & 1 deletion cabal.project
Original file line number Diff line number Diff line change
@@ -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?

8 changes: 4 additions & 4 deletions src/Data/YAML.hs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
{-# LANGUAGE GADTs #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE Safe #-}

-- |
-- Copyright: © Herbert Valerio Riedel 2015-2018
Expand Down Expand Up @@ -103,6 +102,7 @@ import qualified Control.Monad.Fail as Fail
import qualified Data.ByteString as BS
import qualified Data.ByteString.Lazy as BS.L
import qualified Data.Map as Map
import Data.Scientific
import qualified Data.Text as T

import Data.YAML.Dumper
Expand Down Expand Up @@ -473,11 +473,11 @@ instance FromYAML Word32 where parseYAML = parseInt "Word32"
instance FromYAML Word64 where parseYAML = parseInt "Word64"


instance FromYAML Double where
instance FromYAML Scientific where
parseYAML = withFloat "!!float" pure
Comment on lines -476 to 477
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.


-- | Operate on @tag:yaml.org,2002:float@ node (or fail)
withFloat :: String -> (Double -> Parser a) -> Node Pos -> Parser a
withFloat :: String -> (Scientific -> Parser a) -> Node Pos -> Parser a
withFloat _ f (Scalar pos (SFloat b)) = fixupFailPos pos (f b)
withFloat expected _ v = typeMismatch expected v

Expand Down Expand Up @@ -679,7 +679,7 @@ instance Loc loc => ToYAML (Node loc) where
instance ToYAML Bool where
toYAML = Scalar () . SBool

instance ToYAML Double where
instance ToYAML Scientific where
toYAML = Scalar () . SFloat

instance ToYAML Int where toYAML = Scalar () . SInt . toInteger
Expand Down
1 change: 0 additions & 1 deletion src/Data/YAML/Dumper.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE Safe #-}

-- |
-- Copyright: © Herbert Valerio Riedel 2015-2018
Expand Down
1 change: 0 additions & 1 deletion src/Data/YAML/Internal.hs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE Safe #-}

-- |
-- Copyright: © Herbert Valerio Riedel 2015-2018
Expand Down
1 change: 0 additions & 1 deletion src/Data/YAML/Schema.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE Safe #-}
{-# LANGUAGE ScopedTypeVariables #-}

-- |
Expand Down
30 changes: 13 additions & 17 deletions src/Data/YAML/Schema/Internal.hs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE Safe #-}
{-# LANGUAGE ScopedTypeVariables #-}

-- |
Expand Down Expand Up @@ -31,6 +30,7 @@ module Data.YAML.Schema.Internal
import qualified Data.Char as C
import qualified Data.Map as Map
import qualified Data.Set as Set
import Data.Scientific
import qualified Data.Text as T
import Numeric (readHex, readOct)
import Text.Parsec as P
Expand All @@ -42,11 +42,11 @@ import qualified Data.YAML.Event as YE
import Util

-- | Primitive scalar types as defined in YAML 1.2
data Scalar = SNull -- ^ @tag:yaml.org,2002:null@
| SBool !Bool -- ^ @tag:yaml.org,2002:bool@
| SFloat !Double -- ^ @tag:yaml.org,2002:float@
| SInt !Integer -- ^ @tag:yaml.org,2002:int@
| SStr !Text -- ^ @tag:yaml.org,2002:str@
data Scalar = SNull -- ^ @tag:yaml.org,2002:null@
| SBool !Bool -- ^ @tag:yaml.org,2002:bool@
| SFloat !Scientific -- ^ @tag:yaml.org,2002:float@
| SInt !Integer -- ^ @tag:yaml.org,2002:int@
| SStr !Text -- ^ @tag:yaml.org,2002:str@

| SUnknown !Tag !Text -- ^ unknown/unsupported tag or untagged (thus unresolved) scalar
deriving (Eq,Ord,Show,Generic)
Expand Down Expand Up @@ -271,10 +271,10 @@ coreDecodeInt t
--
-- > -? ( 0 | [1-9] [0-9]* ) ( \. [0-9]* )? ( [eE] [-+]? [0-9]+ )?
--
jsonDecodeFloat :: T.Text -> Maybe Double
jsonDecodeFloat :: T.Text -> Maybe Scientific
jsonDecodeFloat = either (const Nothing) Just . parse float ""
where
float :: Parser Double
float :: Parser Scientific
float = do
-- -?
p0 <- option "" ("-" <$ char '-')
Expand Down Expand Up @@ -306,12 +306,12 @@ jsonDecodeFloat = either (const Nothing) Just . parse float ""
--
-- > [-+]? ( \. [0-9]+ | [0-9]+ ( \. [0-9]* )? ) ( [eE] [-+]? [0-9]+ )?
--
coreDecodeFloat :: T.Text -> Maybe Double
coreDecodeFloat :: T.Text -> Maybe Scientific
coreDecodeFloat t
| Just j <- Map.lookup t literals = Just j -- short-cut
| otherwise = either (const Nothing) Just . parse float "" $ t
where
float :: Parser Double
float :: Parser Scientific
float = do
-- [-+]?
p0 <- option "" (("-" <$ char '-') P.<|> "" <$ char '+')
Expand Down Expand Up @@ -449,15 +449,11 @@ coreSchemaEncoder = SchemaEncoder{..}
encodeBool :: Bool -> T.Text
encodeBool b = if b then "true" else "false"

-- | Encode Double
-- | Encode Scientific
--
-- @since 0.2.0
encodeDouble :: Double -> T.Text
encodeDouble d
| d /= d = ".nan"
| d == (1/0) = ".inf"
| d == (-1/0) = "-.inf"
| otherwise = T.pack . show $ d
encodeDouble :: Scientific -> T.Text
encodeDouble d = T.pack . show $ d
Comment on lines +455 to +456
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?


-- | Encode Integer
--
Expand Down
38 changes: 13 additions & 25 deletions tests/Tests.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import Control.Monad
import Control.Applicative
import Data.Scientific
import Data.YAML as Y
import qualified Data.Text as T
import qualified Data.Map as Map
Expand All @@ -21,27 +22,12 @@ roundTripBool b
| b = "true" == outputStr b
| otherwise = "false" == outputStr b

roundTripDouble :: Double -> Double -> Bool
roundTripDouble num denom
| d /= d = ".nan" == outputStr d
| d == (1/0) = ".inf" == outputStr d
| d == (-1/0) = "-.inf" == outputStr d
| otherwise = BS.L.pack (show d) == outputStr d
where d = num / denom

roundTrip :: (Eq a, FromYAML a, ToYAML a) => (a -> a -> Bool) -> a -> a -> Bool
roundTrip eq _ v =
case decode1 (encode1 v) :: (FromYAML a) => (Either (Pos, String) a) of
Left _ -> False
Right ans -> ans `eq` v

approxEq :: Double -> Double -> Bool
approxEq a b = a == b || d < maxAbsoluteError || d / max (abs b) (abs a) <= maxRelativeError
where
d = abs (a - b)
maxAbsoluteError = 1e-15
maxRelativeError = 1e-15

roundTripEq :: (Eq a, FromYAML a, ToYAML a) => a -> a -> Bool
roundTripEq x y = roundTrip (==) x y

Expand All @@ -53,20 +39,22 @@ tests =
[ testGroup "encode"
[ testProperty "encodeInt" roundTripInt
, testProperty "encodeBool" roundTripBool
, testProperty "encodeDouble" roundTripDouble
]
, testGroup "roundTrip"
[ testProperty "Bool" $ roundTripEq True
, testProperty "Double" $ roundTrip approxEq (1::Double)
, testProperty "Int" $ roundTripEq (1::Int)
, testProperty "Integer" $ roundTripEq (1::Integer)
, testProperty "Text" $ roundTripEq T.empty
, testProperty "Seq" $ roundTripEq ([""]:: [T.Text])
, testProperty "Map" $ roundTripEq (undefined :: Map.Map T.Text T.Text)
, testProperty "Foo" $ roundTripEq (undefined :: Foo)
[ testProperty "Bool" $ roundTripEq True
, testProperty "Int" $ roundTripEq (1::Int)
, testProperty "Integer" $ roundTripEq (1::Integer)
, testProperty "Scientific" $ roundTripEq (1::Scientific)
, testProperty "Text" $ roundTripEq T.empty
, testProperty "Seq" $ roundTripEq ([""]:: [T.Text])
, testProperty "Map" $ roundTripEq (undefined :: Map.Map T.Text T.Text)
, testProperty "Foo" $ roundTripEq (undefined :: Foo)
]
]

instance Arbitrary Scientific where
arbitrary = scientific <$> arbitrary <*> arbitrary

instance Arbitrary T.Text where
arbitrary = T.pack <$> arbitrary

Expand Down Expand Up @@ -95,4 +83,4 @@ instance FromYAML Foo where
<*> m .: "fooMap"

instance Arbitrary Foo where
arbitrary = liftM5 Foo arbitrary arbitrary arbitrary arbitrary arbitrary
arbitrary = liftM5 Foo arbitrary arbitrary arbitrary arbitrary arbitrary