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

Potential conversion issues with numbers #15

Open
lwabeke opened this issue Dec 20, 2019 · 1 comment
Open

Potential conversion issues with numbers #15

lwabeke opened this issue Dec 20, 2019 · 1 comment

Comments

@lwabeke
Copy link
Contributor

lwabeke commented Dec 20, 2019

I found some problems in LazyJSON when trying to see if it can interwork with Unmarshal.jl, trying to reconstruct Julia objects from JSON serialization of the objects.

Conversion to Float32 is not supported and conversion to Float64 does a non-standard ?conversion which causes the reconstructed values to differ for some Float64 values.

See below:

julia> using JSON, JSON2, JSON3, LazyJSON, Unmarshal

julia> val = -1.74632
-1.74632

julia> jstring = json(val)
"-1.74632"

julia> j1 = JSON.parse(jstring)
-1.74632

julia> j2 = JSON2.read(jstring)
-1.74632

julia> j3 = JSON3.read(jstring)
-1.74632

julia> jl = LazyJSON.parse(jstring)
-1.74632

julia> typeof(j1), typeof(j2), typeof(j3), typeof(jl)
(Float64, Float64, Float64, LazyJSON.Number{String})

julia> Float64(jl)
-1.7463199999999999

julia> j1 == j2 == j3
true

julia> j1 == j2 == j3 == jl
false

julia> Float32(jl) == Float32(val)
ERROR: MethodError: no method matching Float32(::LazyJSON.Number{String})
Closest candidates are:
  Float32(::Real, ::RoundingMode) where T<:AbstractFloat at rounding.jl:200
  Float32(::T) where T<:Number at boot.jl:718
  Float32(::Int8) at float.jl:60
  ...
Stacktrace:
 [1] top-level scope at REPL[5]:1

It seems that LazyJSON's conversion from LazyJSON.Number{String} to Float64 is different from the other JSON packages.
I did a single line change to support Float32 conversion, which I will submit as a PR. Then the last line works.

@NHDaly
Copy link

NHDaly commented Dec 29, 2020

We ran into this issue recently, and I want to report another two examples of the above:

julia> Float64(LazyJSON.parse("1.7272"))
1.7271999999999998

julia> Float64(LazyJSON.parse("1.727"))
1.7269999999999999

From what I can tell from reading the spec that's linked (https://tools.ietf.org/html/rfc7159#section-6), json is supposed to be able to represent any Float64 value losslessly from julia? Is that how I should read this?:

good interoperability can be
achieved by implementations that expect no more precision or range
than these provide, in the sense that implementations will
approximate JSON numbers within the expected precision


What would you think about changing the custom parse_number() implementation to instead of each of these functions:

LazyJSON.jl/src/Number.jl

Lines 136 to 150 in 53c63f0

Base.Int8(n::JSON.Number)::Int8 = Base.Number(n)
Base.Int16(n::JSON.Number)::Int16 = Base.Number(n)
Base.Int32(n::JSON.Number)::Int32 = Base.Number(n)
Base.Int64(n::JSON.Number)::Int64 = Base.Number(n)
Base.Int128(n::JSON.Number)::Int128 = Base.Number(n)
Base.UInt8(n::JSON.Number)::UInt8 = Base.Number(n)
Base.UInt16(n::JSON.Number)::UInt16 = Base.Number(n)
Base.UInt32(n::JSON.Number)::UInt32 = Base.Number(n)
Base.UInt64(n::JSON.Number)::UInt64 = Base.Number(n)
Base.UInt128(n::JSON.Number)::UInt128 = Base.Number(n)
Base.BigInt(n::JSON.Number)::BigInt = Base.Number(n)
Base.Float32(n::JSON.Number)::Float32 = Base.Number(n)
Base.Float64(n::JSON.Number)::Float64 = Base.Number(n)
Base.BigFloat(n::JSON.Number)::BigFloat = Base.Number(n)
Base.AbstractFloat(n::JSON.Number)::AbstractFloat = Base.Number(n)

be implemented via calls to Base.parse(T, s)?

Something like this?:

for T in (Int8, Int16, #= ..., =# Float64,)
    @eval Base.$T(n::JSON.Number)::$T = Base.parse($T, @view n.s[n.i:end])
end

This way we would ensure that we keep the parse equivalent to how Julia would parse it, and simplify the implementation?


After a bit more reading, I see now that you want a custom parse_number() implementation so that you can handle a number whose expected type you don't yet know in getvalue() and getflat():

elseif isnum(c) JSON.Number(s, i)

elseif isnum(c) parse_number(s, i)

But I think you could keep the current implementation of parse_number() (or maybe also simplify it) and only use it for identifying how many characters the number takes up (which is one of its current functions), but stop using it for conversion (via the eval definition suggestion above), since when converting the user provides the specified type, and in that case we can rely on julia's implementation, AND it's maybe better to repeat the parse with the output type in mind, since that would help support parsing to custom types (e.g. FixedPointDecimal), and might allow us to avoid certain rounding errors or something?

Anyway, I would appreciate your thoughts on this!

Thanks very much.

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

No branches or pull requests

2 participants