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

Sg/area dist #27

Merged
merged 18 commits into from
Dec 24, 2023
Merged

Sg/area dist #27

merged 18 commits into from
Dec 24, 2023

Conversation

skygering
Copy link
Collaborator

Update and fully test the area and distance function.

@skygering skygering requested a review from rafaqz December 21, 2023 22:33
Copy link
Member

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

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

Tests look great.

There are a lot of separate docs... I find it easier to read one doc string on a stub function covering all the methods of the function, rather than many separate per-method docs.

"""
function area(::GI.PointTrait, point)
T = typeof(GI.x(point))
return T(0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return T(0)
return zero(T)

"""
function area(::CT, curve) where CT <: GI.AbstractCurveTrait
T = typeof(GI.x(GI.getpoint(curve, 1)))
return T(0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return T(0)
return zero(T)



"""
_signed_area(geom)::Real
Copy link
Member

Choose a reason for hiding this comment

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

Underscore functions should have comments rather than docs

"""
area(geom)::Real

Returns the area of the geometry.
Copy link
Member

Choose a reason for hiding this comment

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

Probably this function could have all of the area docs.

"""
signed_area(geom)::Real

Returns the signed area of the geometry, based on winding order.
Copy link
Member

Choose a reason for hiding this comment

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

And this one all of the signed_area docs

Comment on lines 150 to 151
area /= 2
return area
Copy link
Member

@rafaqz rafaqz Dec 22, 2023

Choose a reason for hiding this comment

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

Suggested change
area /= 2
return area
return area / 2

@skygering
Copy link
Collaborator Author

skygering commented Dec 22, 2023

There are a lot of separate docs... I find it easier to read one doc string on a stub function covering all the methods of the function, rather than many separate per-method docs.

Okay! I combined all of the docs into the stub function and then organized the others with comments. Let me know if that is easier to read.

@skygering
Copy link
Collaborator Author

Also, the code that was originally in the file had the signed_distance from a point to a multipolygon as the maximum minimum distance from a point to an element of the multipolygon. But based on LibGEOS behavior (as shown in tests) and this description of Shapely behavior, I think it should be the minimum minimum distance from the point to an element of the multipolygon. But let me know what you think.

@rafaqz
Copy link
Member

rafaqz commented Dec 22, 2023

@asinghvi17 what do you think? I have no knowledge of signed distance algs to really comment on that

@rafaqz
Copy link
Member

rafaqz commented Dec 23, 2023

The minimum minimum does seem correct to me too, based on not very much

@skygering
Copy link
Collaborator Author

I feel good about merging it if you do @rafaqz. If someone disagrees with the algorithm we can always fix it later.

@rafaqz rafaqz merged commit 9d24c8d into JuliaGeo:main Dec 24, 2023
3 checks passed
@skygering skygering deleted the sg/area_dist branch January 2, 2024 18:19
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