search

JuliaGeometry / GeometryBasics.jl

Basic Geometry Types
MIT License
165 stars 54 forks source link

Implemented GeoInterface. #175

Closed evetion closed 2 years ago

evetion commented 2 years ago
evetion commented 2 years ago

@sjkelly Is it ok if we bump the minimum required Julia version to 1.6 here? It's a requirements for GeoInterface and since this will be a breaking release, it might be ok?

visr commented 2 years ago

Makie also still has julia 1.3 compat I see (though it seems CI from 1.6). Why would this be breaking by the way? It just adds a feature right?

evetion commented 2 years ago

Well the GeoInterface is a breaking release, so what worked in the old version won't work in the new one. I'd say it's breaking, but it depends on what you consider part of the package.

visr commented 2 years ago

I'd say since the GeoInterface is only added now that it is v1 it is not breaking, for GeometryBasics. Bumping the Julia compat to 1.6 also does not have to be a breaking release these days, right? Since older (unsupported) julia version will just not pick it up

sjkelly commented 2 years ago

I am not opposed to dropping pre-1.6. @SimonDanisch does the next version of Makie targeting 1.3 compat?

SimonDanisch commented 2 years ago

I think we're slowly giving up on 1.3, so fine by me I suppose!

sjkelly commented 2 years ago

@SimonDanisch do you want to check this for load time regressions since it adds a few new dependencies?

SimonDanisch commented 2 years ago

Huh, I guess those aren't due to this PR:

image

But seems like we still need compat bounds for GeoInterface, though!

do you want to check this for load time regressions since it adds a few new dependencies?

I just tried, and if there's one it's barely measurable... Which makes sense, since the dependency is very small, so shouldn't be in the way ;)

sjkelly commented 2 years ago

But seems like we still need compat bounds for GeoInterface, though!

Aqua.jl doing work before registrator denies you :P

visr commented 2 years ago

For the deprecation warnings, they come from Aqua.test_all, I just filed https://github.com/JuliaTesting/Aqua.jl/issues/83.

But seems like we still need compat bounds for GeoInterface, though!

I'll add those plus a few Base.convert methods that will allow us to convert geometry types through the interface, https://juliageo.org/GeoInterface.jl/dev/guides/developer/#Conversion

visr commented 2 years ago

Ok I finished the remaining items, this is good to go.

lazarusA commented 2 years ago

wow! @visr rocks!!!

visr commented 2 years ago

Haha many have worked on this in some way. But I'm glad it's merged, it will definitely help the interoperability of the ecosystems.

using GADM, CairoMakie, GeoInterface
using CairoMakie: MultiPolygon

gdalgeom = only(GADM.get("UKR").geom)
basicgeom = GeoInterface.convert(MultiPolygon, gdalgeom)
poly(basicgeom)

download (Small)