Python implementation for Graham Scan #238
Conversation
wmboyles
changed the title
|
|
||
|
|
||
| #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.
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.
Incosistent naming, see my main main comment
Gathros
added
the
Implementation
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.
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.
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.
dude! You're missing the : this is still fatal! Have you tried running it?
There was a problem hiding this comment.
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.
@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. |
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.