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

GeoJSON to 0.6 and GeoInterface support #123

Merged
merged 10 commits into from
Sep 21, 2022

Conversation

jeremiahpslewis
Copy link
Contributor

@jeremiahpslewis jeremiahpslewis commented Aug 16, 2022

Hey @asinghvi17 @SimonDanisch!

Thanks for the great package. GeoJSON 0.5 was crashing / stalling on my M1 machine (( believe due to an outdated / nonfunctional binary dependency), so I went down the rabbit hole of updating GeoMakie to support GeoJSON 0.6. There is a set of hacks in src/patch.jl and modifications to example plots which are the result of my limited understanding of the GeoInterface / GeometryBasics ecosystem, but the tests pass and I would be happy to put more time into eliminating the hacks and rolling back the example plot modifications if you could point me in the right direction.

Ideally, when finished, this PR will resolve #117.

Cheers, Jeremiah

@jeremiahpslewis jeremiahpslewis changed the title GeoJSON to 0.6 and associated changes GeoJSON to 0.6 and GeoInterface support Aug 16, 2022
@visr
Copy link
Collaborator

visr commented Aug 16, 2022

Thanks for taking this on! I think something like geoJSONtraitParse might be a good addition to GeoInterface.jl, though it's also fine to first have it here. The convert function only creates a user specified geometry type, but if you just want to convert to the matching geometry type of another package, there is no direct support.

The * 1.0 in patches.jl, for which files/examples are they needed? I'm aware that GeoJSON/JSON3 parses [1.0,2.0] as two Ints, which can be annoying. I still have to make separate issues in GeoJSON.jl, but linked to a JSON3 issue in visr/GeoJSONTables.jl#15 (comment). Since GeoJSON is in lat/lon, this should normally not occur very often however.

@jeremiahpslewis
Copy link
Contributor Author

jeremiahpslewis commented Aug 16, 2022

The integer issue occurs in both package datasets (search for [-61.77,10] or [-98.16,75]) as well as in examples/field_and_countries.jl and examples/world_population.jl and examples/italy.jl if I recall correctly. I guess if one randomly samples numbers with 2-3 sig figs, you get land on an integer now and again, which causes JSON array's to have a union type signature Union{Float64, Int64}[180, 68.963646].

One line example of the problem is here, when the patch is commented out (look at the final element):

using GeoInterface, GeoMakie
GeoInterface.coordinates(GeoInterface.geometry.(GeoMakie.coastlines())[94])

Without the fix / hack, you get the following error:

ERROR: MethodError: no method matching GeometryBasics.Polytope(::Type{GeometryBasics.Ngon{Dim, T, 2, P} where {Dim, T, P<:GeometryBasics.AbstractPoint{Dim, T}}}, ::Type{Point2})
Closest candidates are:
  GeometryBasics.Polytope(::Type{<:GeometryBasics.Ngon{Dim, T, N, P} where {Dim, T, P}}, ::Type{<:GeometryBasics.AbstractPoint{NDim, T}}) where {N, NDim, T} at ~/.julia/packages/GeometryBasics/5Sb5M/src/basic_types.jl:89
Stacktrace:
  [1] connect
    @ ~/.julia/packages/GeometryBasics/5Sb5M/src/viewtypes.jl:77 [inlined]
  [2] GeometryBasics.LineString(points::Vector{Point2}, skip::Int64) (repeats 2 times)
    @ GeometryBasics ~/.julia/packages/GeometryBasics/5Sb5M/src/basic_types.jl:209
  [3] convert(#unused#::Type{GeometryBasics.LineString}, type::GeoInterface.LineStringTrait, geom::GeoJSON.LineString{JSON3.Object{Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}}})
    @ GeometryBasics ~/.julia/packages/GeometryBasics/5Sb5M/src/geointerface.jl:88
  [4] convert
    @ ~/.julia/packages/GeoInterface/J298z/src/interface.jl:612 [inlined]
  [5] geoJSONtraitParse(geometry::GeoJSON.LineString{JSON3.Object{Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}}})
    @ GeoMakie ~/.julia/dev/GeoMakie/src/patch.jl:26
  [6] _broadcast_getindex_evalf
    @ ./broadcast.jl:670 [inlined]
  [7] _broadcast_getindex
    @ ./broadcast.jl:643 [inlined]
  [8] getindex
    @ ./broadcast.jl:597 [inlined]
  [9] copyto_nonleaf!(dest::Vector{GeometryBasics.LineString{2, Float64, Point2{Float64}, Base.ReinterpretArray{GeometryBasics.Line{2, Float64}, 1, Tuple{Point2{Float64}, Point2{Float64}}, GeometryBasics.TupleView{Tuple{Point2{Float64}, Point2{Float64}}, 2, 1, Vector{Point2{Float64}}}, false}}}, bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Tuple{Base.OneTo{Int64}}, typeof(GeoMakie.geoJSONtraitParse), Tuple{Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(GeoInterface.geometry), Tuple{Base.Broadcast.Extruded{GeoJSON.FeatureCollection{GeoJSON.Feature{JSON3.Object{Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}}}, JSON3.Object{Base.CodeUnits{UInt8, String}, Vector{UInt64}}, JSON3.Array{JSON3.Object, Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}}}, Tuple{Bool}, Tuple{Int64}}}}}}, iter::Base.OneTo{Int64}, state::Int64, count::Int64)
    @ Base.Broadcast ./broadcast.jl:1055
 [10] copy
    @ ./broadcast.jl:907 [inlined]
 [11] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(GeoMakie.geoJSONtraitParse), Tuple{Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(GeoInterface.geometry), Tuple{GeoJSON.FeatureCollection{GeoJSON.Feature{JSON3.Object{Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}}}, JSON3.Object{Base.CodeUnits{UInt8, String}, Vector{UInt64}}, JSON3.Array{JSON3.Object, Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}}}}}}})
    @ Base.Broadcast ./broadcast.jl:860
 [12] top-level scope
    @ REPL[2]:1

@visr
Copy link
Collaborator

visr commented Aug 16, 2022

Hmm you're right, it seems like the integer issue is quite common, since for every single point there is a risk of it happening, especially as you say with few decimals.

@lazarusA
Copy link
Contributor

Weird. I'm also on a M1 machine (and not crashing or stalling ) and the following versions work ok for examples/italy.jl

Julia 1.8 (ARM (M-series Processor))

pkg> st GeoMakie CairoMakie GeoJSON
Status `~/Desktop/Project.toml`
  [13f3f980] CairoMakie v0.8.13
⌅ [61d90e0f] GeoJSON v0.5.1
  [db073c08] GeoMakie v0.4.2
Info Packages marked with ⌅ have new versions available but cannot be upgraded. To see why use `status --outdated`

italy

@jeremiahpslewis
Copy link
Contributor Author

Thanks for the heads up @lazarusA Just retried on my machine. Still stalls at geo = GeoJSON.read(read(it_states, String)). But it isn't a crucial point for the PR as the idea is to get GeoJSON updated either way. :)

julia> versioninfo()
Julia Version 1.8.0
Commit 5544a0fab76 (2022-08-17 13:38 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin21.3.0)
  CPU: 10 × Apple M1 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-13.0.1 (ORCJIT, apple-m1)
  Threads: 1 on 8 virtual cores
(@v1.8) pkg> st
Status `~/.julia/environments/v1.8/Project.toml`
  [13f3f980] CairoMakie v0.8.13
  [8be319e6] Chain v0.5.0
⌅ [61d90e0f] GeoJSON v0.5.1
  [db073c08] GeoMakie v0.4.2
  [c3e4b0f8] Pluto v0.19.11
  [c94c279d] Proj v1.0.0

@lazarusA
Copy link
Contributor

Yes, having an update will be nice. BTW. This is the env that is working for me:

julia> versioninfo()
Julia Version 1.8.0
Commit 5544a0fab76 (2022-08-17 13:38 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin21.3.0)
  CPU: 8 × Apple M1
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-13.0.1 (ORCJIT, apple-m1)
  Threads: 6 on 4 virtual cores
Environment:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 6

(map_projections) pkg> st
Status `~/Desktop/Julia1dot8/Project.toml`
  [13f3f980] CairoMakie v0.8.13
  [e9467ef8] GLMakie v0.6.13
⌅ [61d90e0f] GeoJSON v0.5.1
  [db073c08] GeoMakie v0.4.2
  [3cb90ccd] RasterDataSources v0.5.4
⌃ [a3a2b9e3] Rasters v0.2.2
  [ade2ca70] Dates
Info Packages marked with ⌃ and ⌅ have new versions available, but those with ⌅ cannot be upgraded. To see why use `status --outdated`

@lazarusA
Copy link
Contributor

lazarusA commented Sep 7, 2022

it looks like this PR is ready @asinghvi17, isn't?

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.

GeoInterface 1 support
4 participants