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

Implemented GeoInterface. #175

Merged
merged 10 commits into from
Aug 4, 2022

Conversation

evetion
Copy link
Contributor

@evetion evetion commented Jun 12, 2022

  • Add some extra tests
  • Slim down implementation/types

@evetion
Copy link
Contributor Author

evetion commented Jun 16, 2022

@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
Copy link
Member

visr commented Jun 16, 2022

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
Copy link
Contributor Author

evetion commented Jun 16, 2022

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
Copy link
Member

visr commented Jun 16, 2022

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
Copy link
Member

sjkelly commented Jun 16, 2022

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

@SimonDanisch
Copy link
Member

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

@evetion evetion marked this pull request as ready for review June 20, 2022 16:36
@sjkelly
Copy link
Member

sjkelly commented Jul 6, 2022

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

@SimonDanisch
Copy link
Member

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
Copy link
Member

sjkelly commented Aug 3, 2022

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

Aqua.jl doing work before registrator denies you :P

@visr
Copy link
Member

visr commented Aug 3, 2022

For the deprecation warnings, they come from Aqua.test_all, I just filed JuliaTesting/Aqua.jl#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

The trait names can get quite long and are exported, and not much else is exported.
Not strictly needed since they trigger AssertionErrors if not ok, but good to count towards the tests since they return true if they don't fail.
@visr
Copy link
Member

visr commented Aug 4, 2022

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

@sjkelly sjkelly merged commit 9b2d1fc into JuliaGeometry:master Aug 4, 2022
@lazarusA
Copy link

lazarusA commented Aug 4, 2022

wow! @visr rocks!!!

@visr
Copy link
Member

visr commented Aug 4, 2022

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)

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.

5 participants