Skip to content

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

Merged
merged 17 commits into from
Jul 20, 2018
Merged

Python implementation for Graham Scan #238

merged 17 commits into from
Jul 20, 2018

Conversation

wmboyles
Copy link
Contributor

@wmboyles wmboyles commented Jul 8, 2018

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.

@wmboyles wmboyles changed the title Python implementation from Graham Scan Python implementation for Graham Scan Jul 8, 2018
Copy link
Contributor

@zsparal zsparal left a 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):
Copy link
Contributor

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):
Copy link
Contributor

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

@Gathros Gathros added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jul 8, 2018


#Sort the list of point by their polar angle
def sort_By_Polar(ref, points):
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

Copy link
Contributor Author

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.

@wmboyles
Copy link
Contributor Author

wmboyles commented Jul 9, 2018

The code still give incorrect hulls if the gift contains duplicate points. Should I include something to remove duplicate points before calculating the hull?

@zsparal
Copy link
Contributor

zsparal commented Jul 9, 2018

Snake case is all lowercase with underscores, this is still different

@Butt4cak3
Copy link
Contributor

So... Is this one good now @Gustorn?

zsparal
zsparal previously approved these changes Jul 19, 2018
Copy link
Member

@jiegillet jiegillet left a 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?

@jiegillet jiegillet self-assigned this Jul 20, 2018
@wmboyles
Copy link
Contributor Author

I changed the files to the specified directories. A conflict still remains, seemingly in the contents of graham_scan.md.

jiegillet
jiegillet previously approved these changes Jul 20, 2018
Copy link
Member

@jiegillet jiegillet left a 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.

@jiegillet jiegillet merged commit 87ca7cb into algorithm-archivists:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants