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

Make centroid use GeoInterface #19

Merged
merged 12 commits into from
Oct 2, 2023
Merged

Make centroid use GeoInterface #19

merged 12 commits into from
Oct 2, 2023

Conversation

skygering
Copy link
Collaborator

This is not done. I still need to write a few more tests / figure out why the multi polygon test is failing. However, I wanted to get feedback as I am still getting the hang of GeoInterface.

Thanks!

@skygering skygering marked this pull request as draft September 28, 2023 04:29
@skygering skygering requested a review from rafaqz September 28, 2023 04:29
@rafaqz
Copy link
Member

rafaqz commented Sep 29, 2023

This looks really good to me, I didnt see anything I would change.

@skygering
Copy link
Collaborator Author

Okay so I added more tests! I also added code I had for a random polygon generator to make it easier for us to create polygons as we go, or even just call the generator in our tests. No guarantees that it doesn't make self-intersecting polygons though (although I changed some things to make it less likely).

The other big change is that I switched from providing signed area to just providing area. It was too complex, especially with multi-polygons (what does signed area even mean for those?). I also had to take the absolute value of the area for calculating polygons and multi-polygons anyways to get the weighting correct.

@skygering skygering marked this pull request as ready for review September 29, 2023 21:53
@skygering
Copy link
Collaborator Author

I also wanted to decide what the output should be. @rafaqz and @asinghvi17 do you envision these as giving back tuples, GeoInterface objects, or a geometry object equal to the type put in? We should try to be consistent across all of the functions.

I also want to get rid of the Float64, but not sure how. Any suggestions?

@rafaqz
Copy link
Member

rafaqz commented Sep 30, 2023

Do you want to use the point type to set the float type?

You could do :

T = typeof(GI.x(getpoint(geom, 1)))

Then zero(T) is probably more correct than T(0).

If you want the type to be equal yo the original type you could do

GI.convert(typof(GI.getpoint(geom, 1)), point)

But its not really guaranteed to work because e.g. Shapefile.jl has no GI.convert methods because they're not usefull objects except when reading a shapefile.

So I dont know what object to return really, it doesnt matter a lot at this stage.

@skygering
Copy link
Collaborator Author

Okay great! I will switch to zero(T) and I think I will just have it return a Tuple then. It seems weird to return a GeoInterface Point when that might not be something that the user wants to deal with. So returning something that is a standard Julia type/object might be better.

Once I make those changes, am I good to merge?

@skygering skygering merged commit 2ab27e4 into main Oct 2, 2023
@skygering skygering deleted the sg/GI_centroid branch October 2, 2023 19:29
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.

2 participants