-
-
Notifications
You must be signed in to change notification settings - Fork 360
Python implementation for Graham Scan #238
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
Python implementation for Graham Scan #238
Conversation
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.
pep8 uses snake_case
for function and variable names, I think we should follow this convention.
|
||
|
||
#Find the point with least y-value. If tie, use point with least x-value. | ||
def minYPoint(points): |
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.
This is just: min(points, key=lambda p: (p[1], p[0]));
from math import atan2 | ||
|
||
#Is the turn counter clockwise? | ||
def CCW(p1, p2, p3): |
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.
Incosistent naming, see my main main comment
This should make the code follow pep-8 standards.
|
||
|
||
#Sort the list of point by their polar angle | ||
def sort_By_Polar(ref, points): |
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.
You can replace your two sorting functions by sorted(ref, key=lambda point: atan2(point[1]-ref[0],point[0]-ref[0]) )
, although defining the atan2
function to polar_angle
might be more readable.
|
||
|
||
def graham_Scan(gift): | ||
start = min(gift, key=lambda pL (p[1],p[0])) #Must be in hull |
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.
Typo here: lambda pL
instead of lambda p:
. This would prevent the code from running at all. Don't commit code that can't run.
|
||
|
||
def graham_Scan(gift): | ||
start = min(gift, key=lambda p (p[1],p[0])) #Must be in hull |
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.
dude! You're missing the :
this is still fatal! Have you tried running it?
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.
I got it now. I don't use lambdas too often.
This typo caused some points be be erroneously included in the hull.
The code still give incorrect hulls if the gift contains duplicate points. Should I include something to remove duplicate points before calculating the hull? |
Snake case is all lowercase with underscores, this is still different |
So... Is this one good now @Gustorn? |
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.
@wmboyles you're right, your algorithm breaks when there are duplicated points. That has to do with the counter_clockwise
not making too much sense when points are degenerate, as far as I can tell. Please do remove duplicates, I suggest gift = list(set(gift))
.
Also, GitHub doesn't let us merge the PR automatically, because it can't deal very well with directory changes. It's out fault, we changed the structure of the repo. chapters/computational_geometry/gift_wrapping/graham_scan/graham_scan.md
should be move to contents/graham_scan/graham_scan.md
. Also the code should be moved to contents/graham_scan/code/python/grahamScan.py
. Can you do that?
I changed the files to the specified directories. A conflict still remains, seemingly in the contents of graham_scan.md. |
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.
All good now, the remaining conflicts could be handled by GitHub's interface.
The code as written works on the example. However, I'd like the helper functions to be a little more understandable for someone trying to see how the code works. Also, this code does not seem to give back correct hulls when duplicate points are in the gift.