search

JuliaGeometry / GeometryBasics.jl

Basic Geometry Types
MIT License
164 stars 54 forks source link

Support heterogeneous features #74

Closed Sov-trotter closed 4 years ago

Sov-trotter commented 4 years ago

I haven't removed the old meta implementation as planned due to some issues with MeshMeta. But for the new implemetation, tests are almost written. We just need to fix the Mesh issue, add some other helpful constructors.

Sov-trotter commented 4 years ago

71

visr commented 4 years ago

@SimonDanisch the basic reason for going down this path is https://github.com/JuliaGeometry/GeometryBasics.jl/issues/49#issuecomment-651203611. This PR is only a draft, but do you in general agree with the proposal of moving from PointMeta to Meta{Point}? Main downside is that Meta{Point} can no longer subtype AbstractPoint.

We are going with the name Feature here which is the GeoJSON concept it corresponds to, and to avoid the clash with Base.Meta, but the name is of course open for discussion. A Feature can be seen as a row in a table. In GeoJSON this table is a FeatureCollection. We could go with StructArray{Feature} here or wrap that in a FeatureCollection struct.

SimonDanisch commented 4 years ago

Main downside is that Meta{Point} can no longer subtype AbstractPoint.

Oh, yeah...that was 100% the reason, that I didn't go with it :D

I think having MetaPoint be an AbstractPoint is pretty important ;) So one way of keeping that would be, to have Meta{X} a fallback, for types, that weren't explicitly created by the macro?

visr commented 4 years ago

I think that would work for us :)

Sov-trotter commented 4 years ago

closes #50 #49

Sov-trotter commented 4 years ago

I think this is ready to be merged? Are there anymore methods that I should add?

SimonDanisch commented 4 years ago

@visr, can you give this a review as well? :)

visr commented 4 years ago

Yes, I'll have a look in a day or two.

Sov-trotter commented 4 years ago

I think GeometryMeta is good to have? Or even MetaT is good since it's synonymous/close to Meta{T}. I think I should add the latter.

Sov-trotter commented 4 years ago

I am extremely sorry for the scattered PR's(both here and in AbstractPlotting). But here's a sequence in which you might want to review/merge them:

  1. Firstly this one

  2. 73

  3. https://github.com/JuliaPlots/AbstractPlotting.jl/pull/479 (Depends on the decompose() method in #73 )
  4. https://github.com/JuliaPlots/AbstractPlotting.jl/pull/486 (Depends on the convert_arguments method for LineString in the above PR)
Sov-trotter commented 4 years ago

Hey @SimonDanisch if the changes are fine then this can be merged?

SimonDanisch commented 4 years ago

Sorry for being a bit slow! Thanks for you contribution!