Skip to content
This repository has been archived by the owner on May 16, 2022. It is now read-only.

[WIP] Expanded SF types to include MM types. #5

Merged
merged 10 commits into from
Jul 29, 2020
Merged

[WIP] Expanded SF types to include MM types. #5

merged 10 commits into from
Jul 29, 2020

Conversation

evetion
Copy link
Member

@evetion evetion commented Mar 23, 2020

Resolves #2 and resolves #3.

@evetion evetion requested a review from visr March 23, 2020 16:51
@visr
Copy link
Member

visr commented Mar 23, 2020

Nice! Would be nice to have focused PRs though, perhaps all those TODOs are a bit much for all in this PR?

@evetion
Copy link
Member Author

evetion commented Apr 5, 2020

Sure. I won't add more implementations/easy checks here. But the Types are there now, including a bounding box one. Let me know what you think. Only thing left is maybe add a method bounds that results in a BoundingBox with a fallback that computes it from all coordinates.

@evetion evetion changed the title [WIP] Added more types. Added all SF types. Added bounding box type. Apr 5, 2020
@evetion evetion mentioned this pull request Apr 5, 2020
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.

I understand the need for a concrete BoundingBox type, and I think this is quite nicely done.

Normally I'd favor using the GeometryBasics HyperRectangle type for this, see this and that. But of course as an interface we cannot take on such a dependency (and we need the reverse dependency).

How do you see the extra complexity of the Line, LinearRing, Triangle, Box (and other) types? Just thinking aloud here. If I want to support Polygon but not the special versions Triangle and Box, I guess an implementation can just cover AbstractPolygon, and use it to create Polygon types?

src/GeoInterfaceRFC.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
src/GeoInterfaceRFC.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.

Could you also update the readme where needed?

visr and others added 3 commits May 16, 2020 21:04
Since `geomtype` is expected to be defined for the instance, not the type itself.
@evetion evetion changed the title Added all SF types. Added bounding box type. Expanded SF types to include MM types. Jun 7, 2020
@evetion
Copy link
Member Author

evetion commented Jun 7, 2020

After some thought I removed the bounding box type. It had some issues, such as the @generated and it doesn't fit in this package as a concrete implementation. I would like to go for an optional extent method that returns something that adheres to a Rectangle type. The Rect type from GeometryBasics would be a good fit.

I've added the CircularCurve and related types from SQL/MM. Also added a lot of defaults, where we can infer that ourselves. I'm ok with NotImplementedErrors on functions where we ourselves could eventually provide a fallback, things like area.

I've also started adding docstrings and did a split for types and interfaces, as requested by @rafaqz in #2.

Some points that still need to be addressed:

  • Do we need to AbstractX everywhere? Is a Triangle <: Polygon or <:AbstractPolygon?
  • I propose a minimal required interface, only geomtype, ncoords, ngeom, getgeom. We can make npoint = ngeom in the case of Points etc. Even Polygons, with exterior and interior(s) can be resolved to nrings (made an example) and thus ngeoms.
  • getgeom can be an iterator.

Still some work to do, but let me know your thoughts on this direction.

@evetion evetion changed the title Expanded SF types to include MM types. [WIP] Expanded SF types to include MM types. Jun 7, 2020
@evetion evetion requested a review from visr June 7, 2020 16:04
src/defaults.jl Outdated Show resolved Hide resolved
src/defaults.jl Outdated Show resolved Hide resolved
src/interface.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
src/GeoInterfaceRFC.jl Outdated Show resolved Hide resolved
@evetion
Copy link
Member Author

evetion commented Jul 29, 2020

  • Updated the exports and the Project.toml
  • Updated the README.md to work with ngeom and getgeom
  • Removed the primitives.jl for now (not included it), as it needs rethinking
  • Made a test implementation as test
  • Made a testing tool to check your implementation

I'd say, barring obvious errors or typos, it's ready to merge. It isn't done yet, as the test suite needs expanding, especially for the default cases, but it's good enough for JuliaCon.

evetion and others added 2 commits July 29, 2020 23:03
is the error that this seems to give on CI on Julia 1.0
@visr visr merged commit df62a18 into master Jul 29, 2020
@visr visr deleted the more-types branch July 29, 2020 21:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review Doc strings?
3 participants