JuliaGeo / LibGEOS.jl

Julia package for manipulation and analysis of planar geometric objects
MIT License
72 stars 23 forks source link

add LibGEOSMakie #144

Closed jw3126 closed 1 year ago

jw3126 commented 1 year ago

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

jw3126 commented 1 year ago

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.

yeesian commented 1 year ago

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.

evetion commented 1 year ago
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.

jw3126 commented 1 year ago

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:

What would you guys prefer?

evetion commented 1 year ago

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.

rafaqz commented 1 year ago

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?

visr commented 1 year ago

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 commented 1 year ago

@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.

rafaqz commented 1 year ago

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?

visr commented 1 year ago

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.

jw3126 commented 1 year ago

@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!

visr commented 1 year ago

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)