-
Notifications
You must be signed in to change notification settings - Fork 4
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
Sg/area dist #27
Conversation
Sg/update intersects
There was a problem hiding this 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.
src/methods/area.jl
Outdated
""" | ||
function area(::GI.PointTrait, point) | ||
T = typeof(GI.x(point)) | ||
return T(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return T(0) | |
return zero(T) |
src/methods/area.jl
Outdated
""" | ||
function area(::CT, curve) where CT <: GI.AbstractCurveTrait | ||
T = typeof(GI.x(GI.getpoint(curve, 1))) | ||
return T(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return T(0) | |
return zero(T) |
src/methods/area.jl
Outdated
|
||
|
||
""" | ||
_signed_area(geom)::Real |
There was a problem hiding this comment.
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
src/methods/area.jl
Outdated
""" | ||
area(geom)::Real | ||
|
||
Returns the area of the geometry. |
There was a problem hiding this comment.
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.
src/methods/area.jl
Outdated
""" | ||
signed_area(geom)::Real | ||
|
||
Returns the signed area of the geometry, based on winding order. |
There was a problem hiding this comment.
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
src/methods/area.jl
Outdated
area /= 2 | ||
return area |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
area /= 2 | |
return area | |
return area / 2 |
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. |
Also, the code that was originally in the file had the |
@asinghvi17 what do you think? I have no knowledge of signed distance algs to really comment on that |
The minimum minimum does seem correct to me too, based on not very much |
I feel good about merging it if you do @rafaqz. If someone disagrees with the algorithm we can always fix it later. |
Update and fully test the area and distance function.