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

add LibGEOSMakie

Open jw3126 opened this issue 3 years ago • 11 comments

https://github.com/JuliaGeo/LibGEOS.jl/issues/142

jw3126 avatar Nov 06 '22 21:11 jw3126

Yes I also don't like how much boilerplate is needed around this line of code to package it. From the user's POV I think

using LibGEOSMakie

is much better than being asked to do some non-standard rituals like

import GeoInterfaceMakie
GeoInterfaceMakie.@enable(LibGEOS.AbstractGeometry)

Also if we ever need to update this, say (@enable(More stuff)) it is much easier to update a single package than every user's script.

If the user does her own

module LibGEOSMakie
import LibGEOS
import GeoInterfaceMakie
GeoInterfaceMakie.@enable(LibGEOS.AbstractGeometry)
end

that is only useful if it can be accessed from multiple projects. Which means it needs to be packaged. Better package it once here than multiple times by users.

jw3126 avatar Nov 08 '22 07:11 jw3126

Also if we ever need to update this, say (@enable(More stuff)) it is much easier to update a single package than every user's script.

That's a fair argument, thanks for answering my questions. I'm okay with merging this PR if @visr is okay with it.

yeesian avatar Nov 09 '22 00:11 yeesian

using LibGEOSMakie

is much better than being asked to do some non-standard rituals like

import GeoInterfaceMakie
GeoInterfaceMakie.@enable(LibGEOS.AbstractGeometry)

I don't think one extra line is a ritual or exactly non-standard. For example, if you want to plot using Plots and PyPlot as backend, you have to do pyplot().

Also if we ever need to update this, say (@enable(More stuff)) it is much easier to update a single package than every user's script.

If the user does her own

module LibGEOSMakie
import LibGEOS
import GeoInterfaceMakie
GeoInterfaceMakie.@enable(LibGEOS.AbstractGeometry)
end

that is only useful if it can be accessed from multiple projects. Which means it needs to be packaged. Better package it once here than multiple times by users.

We are not in the business of updating user scripts, behavior of scripts should stay identical actually. @enable(More stuff) would imply a new feature of a script, so that's fine. We control GeoInterfaceMakie, so any bugs (or even adding More stuff) in enable can be done by us.


Now, I don't want to ignore your point that you would like "just to work" once you do using GeoInterfaceRecipes/Makie. I agree with that, but I'm not sure yet how to implement it yet.

Could we add wrappertypes in GeoInterface? Say the following for any custom {T} geometry that implements geomtrait(T)=PolygonTrait()?

struct PolygonWrap{T}
	geom::{T}
end

Then GeoInterfaceRecipes/Makie could just provide recipes based on those types. Not sure about the performance.

evetion avatar Nov 11 '22 09:11 evetion

Thanks, @evetion ! I agree would be great to have a better approach. My suspicion is however that we won't come up with something better any time soon, while I need this functionality now.

I see two options:

  • We merge this PR and deprecate the package later, should we ever come up with a better mechanism
  • I release this package under jw3126/LibGEOSMakie.jl so you don't need to maintain it at all.

What would you guys prefer?

jw3126 avatar Nov 11 '22 09:11 jw3126

I release this package under jw3126/LibGEOSMakie.jl so you don't need to maintain it at all.

If you need this now, this is of course the best option. I'm on the fence for merging it and deprecating it later, maybe @rafaqz could chime in.

evetion avatar Nov 11 '22 09:11 evetion

It seems pretty annoying for users to have to do anything at all to plot geometries. LibGEOS can't take a dependency on GeoInterfaceMakie.jl?

rafaqz avatar Nov 11 '22 10:11 rafaqz

LibGEOS can't take a dependency on GeoInterfaceMakie.jl?

That was @jw3126's original proposal in #142. As mentioned there the biggest dependency it woul pull in would be GeometryBasics. That seemed a bit much to me but actually maybe it's fine, especially if it's going to shed weight with https://github.com/JuliaGeometry/GeometryBasics.jl/pull/173/files#diff-72ed386c2a0cd1d23c0968297e70023ed98c22490d146dd89fc91f48369bad4d, for instance dropping the StaticArrays dependency.

visr avatar Nov 11 '22 10:11 visr

@jw3126 I see you've set up https://github.com/jw3126/LibGEOSMakie.jl, which is pending registration. Shall we close this PR then for now? If we want we can move it in the subdirectory of this repo later.

Do you want to add a mention of LibGEOSMakie to the LibGEOS readme? And we can mention GeoInterfaceRecipes for Plots.jl users.

visr avatar Nov 19 '22 10:11 visr

Is it really too large a dependency to just include here? (Not as a subpackage) MakieCore.jl only depends on Observables.jl (which has no deps). @visr are you thinking this still depends on Makie.jl?

rafaqz avatar Nov 19 '22 10:11 rafaqz

No I'm aware, that's why I posted https://github.com/JuliaGeo/LibGEOS.jl/pull/144#issuecomment-1311501323. It's mainly GeometryBasics. Though now that LibGEOSMakie exists, perhaps that is fine.

visr avatar Nov 19 '22 12:11 visr

@jw3126 I see you've set up https://github.com/jw3126/LibGEOSMakie.jl, which is pending registration. Shall we close this PR then for now? If we want we can move it in the subdirectory of this repo later.

Feel free to close the PR, but maybe leave an issue open for making Makie + LibGEOS work out of the box?

Do you want to add a mention of LibGEOSMakie to the LibGEOS readme?

Great idea!

jw3126 avatar Nov 19 '22 12:11 jw3126

I'll close this since https://github.com/jw3126/LibGEOSMakie.jl/ is registered and mentioned in the readme here: https://github.com/JuliaGeo/LibGEOS.jl#ecosystem

In the future we can also consider using extension packages that are part of julia 1.9 (to be released): https://pkgdocs.julialang.org/dev/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions)

visr avatar Dec 19 '22 10:12 visr