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

Geometry return types #45

Open
skygering opened this issue Jan 12, 2024 · 17 comments
Open

Geometry return types #45

skygering opened this issue Jan 12, 2024 · 17 comments

Comments

@skygering
Copy link
Collaborator

I wanted to start a conversation about what return types of functions that create geometries should be. For example, simplify, if given a LibGEOS.Polygon as an input, still returns a GeoInterface.Polygon. This can be kinda irritating for users. This problem becomes even more pronounced with things like union, difference, and intersection, which my intern is working on a PR for (probably in the next week). Should they all return GI shapes? Should they return coordinates (and a trait type maybe)? Should they rebuild the same type as the input? What do we think is best?

Would love your opinions @rafaqz @asinghvi17.

@rafaqz
Copy link
Member

rafaqz commented Jan 13, 2024

Would it be so irritating? LibGeos will still accept that as input.

One problem with returning the same type is it's inneficient. Currently we could only do it by calling GI.convert on our return values. And we dont even have GI.convert at all for features.

To get around the cost we would need to write rebuild methods for all geometry types of all packages in extensions to avoid dependencies or more generically, define consistent constructors per trait in each of packages as part of following GeoInterface.

We just dont yet have a dependency free way to make package specific geometries from a vector of tuples except wrapping it as a GI shape and calling GI.convert

I would also suggest we shouldnt return C objects no matter what the input is...

@rafaqz
Copy link
Member

rafaqz commented Jan 13, 2024

With union etc, why not GI shapes? Why would coordinates be better?

@skygering
Copy link
Collaborator Author

Oh my gosh, I didn't know that LibGEOS could take in GI shapes. That is a game-changer! I agree that we shouldn't return C objects. So that throws out returning the input type.

I am hoping to minimize type instability. Obviously, there will be some just due to the nature of these problems. I was thinking if we returned the same coordinate form always, it wouldn't be type unstable. But then it is up to the user to untangle the type, which also doesn't seem great.

Any thoughts about how to minimize type instability?

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

Yeah its pretty nice and tivial to implement now we have GeoInterface.convert.

apply is not actually amazing for type stability currently, we need to rework the anonymous functions somehow. It doesnt matter when the applied function is expensive but can a lot if its cheap.

Type stability is really case by case if you have some examples where its bad we can analyze it.

@skygering
Copy link
Collaborator Author

Or I guess like in my code. If I take the intersection of two polygons and then take the area of the result. Julia has no idea what is returned from intersection, so I would assume the area is going to be run-time evaluated. That sort of thing. I guess I might have an intermediate function that turns the result into a vector of polygons because I only care about intersections that form polygons. But I imagine most people are running multiple geometry functions in their program and functions that return any type of geometry seem problematic. But maybe that is just unavoidable when doing these types of calculations. Thoughts?

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

Except for deep nested recursion in apply I would assume every function here can be type stable unless the passed in geometry object itself isnt.

At worst it should be small union instabilities.

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

Do you use ProfileView.jl and Cthulhu.jl to look into instability?

@skygering
Copy link
Collaborator Author

I have yeah! I probably need to more though!

I guess I don't see how things like intersection won't be a union of all potential types. We also probably need to figure out what we want to return if there is no intersection... Should there be an empty geometry type, or nothing?

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

Can intersection of two polygons return a line or point? Or just another polygon (that maybe does only cover a line or single point)?

@skygering
Copy link
Collaborator Author

In LibGEOS, it can return a Point, a Line, a Polygon, a MultiPolygon, even a GeometryCollection or an Empty Polygon.

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

Really? Ok yes that cant ever be type stable.

We can at least list the possible types manually in a Union I guess...

@skygering
Copy link
Collaborator Author

We could do something different. I am not sure of a good general solution though.

My solution right now is that everything that comes out of those functions is run through a get_polygons function that returns a vector of polygons since that is all that I use, but obviously, that isn't what everyone needs.

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

We could have a keyword where you pass the traits you want, which would skip around the type instability

@skygering
Copy link
Collaborator Author

Ah! That would be so nice. Then I don't have to have the stupid extra function haha. What do you think is a good return type if there aren't any? Nothing? An empty geometry object?

@rafaqz
Copy link
Member

rafaqz commented Jan 17, 2024

If it would be a vector of polygons otherwise it can be an empty vector? Then it can be totally type stable given a trait.

@skygering
Copy link
Collaborator Author

I feel like we should have an option to do that and maybe an option that is type unstable and returns what LibGEOS returns. Or can we just not do that since type instability is such a Julia sin?

@rafaqz
Copy link
Member

rafaqz commented Jan 18, 2024

I think both is best

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

2 participants