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

Support heterogeneous features #74

Merged
merged 28 commits into from
Aug 14, 2020

Conversation

Sov-trotter
Copy link
Contributor

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

#71

@Sov-trotter Sov-trotter marked this pull request as draft July 28, 2020 15:31
@visr
Copy link
Member

visr commented Aug 3, 2020

@SimonDanisch the basic reason for going down this path is #49 (comment). 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
Copy link
Member

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

visr commented Aug 3, 2020

I think that would work for us :)

@Sov-trotter Sov-trotter changed the title Meta Redesign Support heterogeneous features Aug 3, 2020
@Sov-trotter
Copy link
Contributor Author

closes #50 #49

@Sov-trotter Sov-trotter marked this pull request as ready for review August 4, 2020 11:16
@Sov-trotter
Copy link
Contributor Author

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

@SimonDanisch SimonDanisch requested a review from visr August 4, 2020 11:21
@SimonDanisch
Copy link
Member

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

@visr
Copy link
Member

visr commented Aug 5, 2020

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

Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, left a few comments.

Since this will now only be an extension of the existing Meta types, I don't think Feature is a good name anymore. Meta is not an option due to Base.Meta. Alternatives include MetaT (like Triangle / TriangleP) or GeometryMeta / AnyMeta which goes more with PointMeta / PolygonMeta.

src/metadata.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
@Sov-trotter
Copy link
Contributor Author

Sov-trotter commented Aug 7, 2020

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.

src/GeometryBasics.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
src/metadata.jl Outdated Show resolved Hide resolved
Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@visr visr requested a review from SimonDanisch August 8, 2020 08:43
@Sov-trotter
Copy link
Contributor Author

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. Give out points of geometries #73
  3. Overload convert_arguments for LineString JuliaPlots/AbstractPlotting.jl#479 (Depends on the decompose() method in Give out points of geometries #73 )
  4. Support GeometryBasics Polygon JuliaPlots/AbstractPlotting.jl#486 (Depends on the convert_arguments method for LineString in the above PR)

@Sov-trotter Sov-trotter mentioned this pull request Aug 13, 2020
@Sov-trotter
Copy link
Contributor Author

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

@SimonDanisch SimonDanisch merged commit 52f478b into JuliaGeometry:master Aug 14, 2020
@SimonDanisch
Copy link
Member

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

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.

3 participants