JSON3.jl icon indicating copy to clipboard operation
JSON3.jl copied to clipboard

Generated types not working for floats

Open dchansen opened this issue 3 years ago • 7 comments
trafficstars

JSON3 casts all floating points to integers, as long as it can be done without loss of precision. I.e.

test = "{ \"A\": 0.0 }"
x = JSON3.read(test)

JSON3.Object{Base.CodeUnits{UInt8, String}, Vector{UInt64}} with 1 entry:
  :A => 0

However this behaviour causes generated types to not work correctly:

JSON3.@generatetypes test
JSON3.read(test,JSONTypes.Root)

ERROR: ArgumentError: invalid JSON at byte position 9 while parsing type Main.JSONTypes.Root: ExpectedComma
{ "A": 0.0 }

dchansen avatar Dec 21 '21 14:12 dchansen

@quinnj It looks like the generated struct looks like

mutable struct Root
  A::Int64
end

which seems reasonable to me, is this an error with the reader?

mcmcgrath13 avatar Dec 21 '21 15:12 mcmcgrath13

I think the reader is right, and the struct should have had Float64 field. At least, that would have been the desired behavior in my cases, and I think it's reasonable not assume the presence of a decimal point to indicate a floating point number.

dchansen avatar Dec 21 '21 18:12 dchansen

Because the reader says this is an Int64, generate types gets the type information from that. If the examples are all safely castable to Int, that's what generate types will see and propose as the type for the struct.

mcmcgrath13 avatar Dec 21 '21 21:12 mcmcgrath13

This has been brought up a few times; I think probably the best way forward is to do one of two things, or maybe a combination of both:

  • while reading, if we parse any number with "float-specific" characters (., e, etc.), we should not try to cast to integer
  • we could provide a keyword argument that would control how numbers are read; perhaps something like numbertype=BigFloat where you could provide the number type we should parse any number as. That way you could do numbertype=Float64 if desired, with the default being numbertype=nothing, which would represent the current behavior of trying to parse an integer, falling back to float otherwise.

quinnj avatar Dec 22 '21 20:12 quinnj

I think I like option 1.

In this case we know that the number can be read as an Int, but when we try to read into an Int field on the struct, it errors. Any theories?

mcmcgrath13 avatar Dec 22 '21 21:12 mcmcgrath13

I do think that types have to be preserved. That is why I support the first option, too.

mo8it avatar Jan 21 '22 21:01 mo8it

if we parse any number with "float-specific" characters (., e, etc.), we should not try to cast to integer

To JSON 1 and 1.0 is exactly the same number. From a Julia perspective this might look like a good idea, but a JSON minifier will happily cut off the .0, and thereby change the way it is parsed in JSON3, which might not be intended.

The current approach of trying to cast to integer is giving some related but different issues. For GeoJSON.jl we now lazily parse using JSON3.read(source). We don't use the struct API, because it is fast, and can be used as a starting point to convert geometries, which are nested JSON3.Array with a length 2 array for coordinates at the lowest level, to more functional geometry types from other packages. However now whenever a longitude falls on a whole number, interoperability is reduced since other packages cannot handle mixed-type coordinates (link). So the proposed option of numbertype=Float64 would be much more useful for this case. I'd even argue it should be the default, but that would be quite a breaking change, so perhaps would be better to avoid.

visr avatar Aug 16 '22 20:08 visr