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

Add more primitive geometries #104

Closed
juliohm opened this issue Aug 19, 2020 · 10 comments
Closed

Add more primitive geometries #104

juliohm opened this issue Aug 19, 2020 · 10 comments

Comments

@juliohm
Copy link
Member

juliohm commented Aug 19, 2020

Some of our current algorithms rely on operations involving primitive geometries like rectangles and spheres. We should implement some existing interface for these primitives to increase interoperability with other parts of the Julia ecosystem.

@evetion could you please advise on the current status of packages defining primitive geometries and interfaces? I know you've been working on the GeoInterfaceRFC.jl package, and I wonder if it is the way to go nowadays.

Could you also comment on the relationship between GeoInterfaceRFC.jl and GeometryBasics.jl? Will the latter implement the former? Where the actual geometry primitives will live? Can you confirm that these efforts are not limited to 2D spaces?

P.S.: what is the RFC suffix?

@evetion
Copy link

evetion commented Aug 20, 2020

I'd say that GeometryBasics.jl is the way to go, it defines concrete geometry types. See basic_types.jl for more details.

GeoInterface.jl defined only concrete implementations first as well, but the RFC (request for comments) mainly has traits, so there can be interop/fallbacks between the various concrete implementations. I expect the RFC to be dropped eventually and be released as GeoInterface v2.0, and GeometryBasicswill implement that interface as well. At the moment Shapefile.jl is also implementing GeometryBasics.jl.

Pinging @visr, in case I forgot something, he's pushing most of the integration work. 👍

@visr
Copy link
Contributor

visr commented Aug 20, 2020

That's right, and indeed these efforts are not limited to 2D spaces.

@visr
Copy link
Contributor

visr commented Aug 20, 2020

Not sure if you've already seen it, but I also tried to explain the situation a bit in my JuliaCon 2020 talk: https://youtu.be/wih_DIWODUs.

So in short, I would say use the primitives from GeometryBasics.jl, unless you have a strong reason not to. In that case, you could use other geometry types, and as long as they implement the new GeoInterface, there should still be easy interop with the rest of the ecosystem.

@juliohm
Copy link
Member Author

juliohm commented Aug 20, 2020

I like that the source code in GeoInterfaceRFC.jl is well-structured and organized, it makes a huge difference when we consider adding a dependency to a project. I am still confused reading the source code of GeometryBasics.jl though. For example, the basic_types.jl file you linked @evetion has a set of primitives but it does not contain simple primitives like rectangle, circle, these live in another file geometry_primitives.jl. I don't feel comfortable yet to add GeometryBasics.jl as a dependency as it is been mainly developed with Makie.jl in mind as opposed to a more general set of primitives to be reused across the ecosystem.

I am thinking of writing a set of primitives in GeoStatsBase.jl adhering to the GeoInterfaceRFC.jl API while the work in GeometryBasics.jl settles down. Do you have plans to drop the RFC suffix and make it the new GeoInterface.jl anytime soon? How should we proceed here?

@visr
Copy link
Contributor

visr commented Aug 20, 2020

I don't feel comfortable yet to add GeometryBasics.jl as a dependency as it is been mainly developed with Makie.jl in mind as opposed to a more general set of primitives to be reused across the ecosystem.

I can assure you @SimonDanisch did exactly design it as a general set of primitives to be reused across the ecosystem. I'm not sure why you are concerned that the types are in two different source files though? Perhaps if you think it can be more neatly arranged you could make a pull request. Whilst working on integrating it with JuliaGeo packages we are also making some pull requests, but no fundamental changes, the basis is pretty solid.

Regarding making GeoInterfaceRFC.jl the new GeoInterface, I hope that we'll be able to do that over the next month or so? If you still have comments on it, now would be a great time :)

@juliohm
Copy link
Member Author

juliohm commented Aug 20, 2020

I'm not sure why you are concerned that the types are in two different source files though?

It leads to errors downstream like people updating things in one file and forgetting to update in the other, etc. Consider for example the naming of primitives, which is a bit off with the "Hyper" prefix sometimes used, but not always used. These small inconsistencies are painful to maintain over time, and I was not sure if PRs changing these tiny details would be welcome. I can try to spot them and submit PRs if they are welcome.

the basis is pretty solid.

Can we compute the volume of any primitive for example? Or the area of the boundary (edges in 2D primitives and surfaces in 3D primitives)? I couldn't figure out if this is available by just reading the source code, and that is a problem IMO. Perhaps these basic primitives could all live in a single file, and the function names like volume, boundary, could live in GeoInterfaceRFC.jl?

Regarding making GeoInterfaceRFC.jl the new GeoInterface, I hope that we'll be able to do that over the next month or so? If you still have comments on it, now would be a great time :)

I will try to digest the proposal today or tomorrow and will open issues there if I find something that is not clear or that could be improved.

@juliohm
Copy link
Member Author

juliohm commented Aug 20, 2020

Opened an issue there: JuliaGeo/GeoInterfaceRFC.jl#6

@juliohm
Copy link
Member Author

juliohm commented Aug 22, 2020

Closing this one and continuing the discussion in GeoInterfaceRFC.jl

@juliohm juliohm closed this as completed Aug 22, 2020
@yeesian
Copy link

yeesian commented Aug 24, 2020

I came across this issue from JuliaGeo/GeoInterfaceRFC.jl#6. My recommendation will be that of @visr's in #104 (comment):

So in short, I would say use the primitives from GeometryBasics.jl, unless you have a strong reason not to.

The primitives in GeometryBasics.jl will have much stronger support for 3D modelling than the proposed interface in GeoInterfaceRFC.jl, which is more appropriate for interoperability of GeometryBasics with drivers in GDAL/GeoJSON/Shapefile/etc.

@evetion
Copy link

evetion commented Aug 24, 2020

I am thinking of writing a set of primitives in GeoStatsBase.jl adhering to the GeoInterfaceRFC.jl API while the work in GeometryBasics.jl settles down. Do you have plans to drop the RFC suffix and make it the new GeoInterface.jl anytime soon? How should we proceed here?

I just made a PR (JuliaGeo/GeoInterface.jl#33) for this. Still needs some working, but we're getting there.

However, as discussed above, I hope you can use/improve GeometryBasics.jl, for the sake of unified geometry approach in Julia.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants