implemented graham scan in C plus plus #616

AkashDhiman · 2019-06-21T00:19:37Z

I also took the edge case when the algorithm can encounter multiple points with the same polar angle or same minimum y co-ordinate, and sorting is done via cross product itself so I didn't include trigonometric functions to do sorting. this is my first PR, let me know if there is something I can improve upon.

dovisutu

Thanks for your contribution. I'm not a really reviewer, but I reviewed it because I know a quite about c++.
You can correct these things, or you can also wait for the real reviewing.
Thankfully, your code does not have compiling problems (unlike tree_traversal before), but there's a runtime error I didn't mention it here. Maybe tomorrow I'll test it and try to add it?

dovisutu · 2019-06-22T14:26:56Z

contents/graham_scan/code/c++/graham_scan.cpp

+
+point origin;
+
+void swap(point &p1, point &p2){


There's a swap function swap<Type>(Type& left, Type& right) in <algorithm> header. Don't make your own.

dovisutu · 2019-06-22T14:39:41Z

contents/graham_scan/code/c++/graham_scan.cpp

+  p2 = temp;
+}
+
+int distSq(point p1, point p2){


As compare is useless, and it only uses in compare, this should also be useless.

dovisutu · 2019-06-22T15:07:11Z

contents/graham_scan/code/c++/graham_scan.cpp

+  double y;
+};
+
+point origin;


Don't use a global origin variable. Localize it in grahamScan and as argument of ccwCheck.

Suggested change

point origin;

dovisutu · 2019-06-22T15:12:04Z

contents/graham_scan/code/c++/graham_scan.cpp

+  return ( crossProduct > 0)? -1 : 1;
+}
+
+int compare(const void *vp1, const void *vp2){


The compare function is totally useless, I think. It's OK to have points colinear, as the one inwards will be replaced.

dovisutu · 2019-06-22T15:17:06Z

contents/graham_scan/code/c++/graham_scan.cpp

+  {
+    return 0;
+  }
+  return ( crossProduct > 0)? -1 : 1;


It's unnececcary to return 1,0 or -1. It's not a bool, so we have to compare it again, therefore, why not just return it?
Also, the previous if is useless, too.

Suggested change

return ( crossProduct > 0)? -1 : 1;

return (b.y - a.y) * (c.x - b.x) - (b.x - a.x) * (c.y - b.y);

dovisutu · 2019-06-22T15:31:58Z

contents/graham_scan/code/c++/graham_scan.cpp

+  origin = points[0];
+
+  //sorting by polar angle and removing duplicates
+  qsort(&points[1], points.size() - 1, sizeof(point), compare);


I prefer to use sort<_Ranit, _Pr>(const _RanIt _First, const _RanIt _Last, _Pr _Pred) in <algorithm> instead of qsort(void* _Base, size_t _NumOfElements, size_t _SizeOfElements, int (*_CompareFunction)(const void*, const void*) in <corecrt_search.h>.
Also, since compare is useless, the _Pr can be a simple lambda [&](point& a, point& b){return ccwCheck(points[0],a,b)

dovisutu · 2019-06-22T15:34:02Z

contents/graham_scan/code/c++/graham_scan.cpp

+
+  //sorting by polar angle and removing duplicates
+  qsort(&points[1], points.size() - 1, sizeof(point), compare);
+  std::vector<point> pointsSorted;


You don't needed to create new one. original one is OK to edit

dovisutu · 2019-06-22T15:45:02Z

contents/graham_scan/code/c++/graham_scan.cpp

+  }
+
+  //creating the convex hull
+  std::vector<point> hull;


As mentioned, the algorithm shouldn't create a new vector to store hull.

To save memory and expensive append() operations, we ultimately look for points that should be on the hull and swap them with the first elements in the array. If there are Melements on the hull, then the first M elements in our output random distribution of points will be the hull.

AkashDhiman · 2019-06-22T17:46:10Z

Thank you for considering to look at my code for graham scan in c++, I will try to improve upon the things you mentioned and will modify the code accordingly as soon as possible.

…

On Sat, 22 Jun 2019, 9:22 pm dovisutu, ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for your contribution. I'm not a really reviewer, but I reviewed it because I know a quite about c++. You can correct these things, or you can also wait for the *real* reviewing. Thankfully, your code does not have compiling problems (unlike tree_traversal before), but there's a runtime error I didn't mention it here. Maybe tomorrow I'll test it and try to add it? ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > @@ -0,0 +1,102 @@ +#include<iostream> +#include<vector> + +struct point { + double x; + double y; +}; + +point origin; + +void swap(point &p1, point &p2){ There's a swap function swap<Type>(Type& left, Type& right) in <algorithm> header. Don't make your own. ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > +#include<vector> + +struct point { + double x; + double y; +}; + +point origin; + +void swap(point &p1, point &p2){ + point temp = p1; + p1 = p2; + p2 = temp; +} + +int distSq(point p1, point p2){ As compare is useless, and it only uses in compare, this should also be useless. ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > @@ -0,0 +1,102 @@ +#include<iostream> +#include<vector> + +struct point { + double x; + double y; +}; + +point origin; Don't use a global origin variable. Localize it in grahamScan and as argument of ccwCheck. ⬇️ Suggested change -point origin; ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > + +int distSq(point p1, point p2){ + return (p1.x - p2.x)*(p1.x - p2.x) +(p1.y - p2.y)*(p1.y - p2.y); +} + +int ccwCheck(point a, point b, point c){ + + int crossProduct = (b.y - a.y) * (c.x - b.x) - (b.x - a.x) * (c.y - b.y); + if (crossProduct == 0) + { + return 0; + } + return ( crossProduct > 0)? -1 : 1; +} + +int compare(const void *vp1, const void *vp2){ The compare function is totally useless, I think. It's OK to have points colinear, as the one inwards will be replaced. ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > + p1 = p2; + p2 = temp; +} + +int distSq(point p1, point p2){ + return (p1.x - p2.x)*(p1.x - p2.x) +(p1.y - p2.y)*(p1.y - p2.y); +} + +int ccwCheck(point a, point b, point c){ + + int crossProduct = (b.y - a.y) * (c.x - b.x) - (b.x - a.x) * (c.y - b.y); + if (crossProduct == 0) + { + return 0; + } + return ( crossProduct > 0)? -1 : 1; It's unnececcary to return 1,0 or -1. It's not a bool, so we have to compare it again, therefore, why not just return it? Also, the previous if is useless, too. ⬇️ Suggested change - return ( crossProduct > 0)? -1 : 1; + return (b.y - a.y) * (c.x - b.x) - (b.x - a.x) * (c.y - b.y); ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > +std::vector<point> grahamScan(std::vector<point> points){ + //selecting origin(the element with y minimum) + double yMin = points[0].y; + int min = 0; + for (size_t i = 1; i < points.size(); i++) { + double y = points[i].y; + if ((y < yMin) || (yMin == y && points[i].x < points[min].x)){ + yMin = points[i].y; + min = i; + } + } + swap(points[0], points[min]); + origin = points[0]; + + //sorting by polar angle and removing duplicates + qsort(&points[1], points.size() - 1, sizeof(point), compare); I prefer to use sort<_Ranit, _Pr>(const _RanIt _First, const _RanIt _Last, _Pr _Pred) in <algorithm> instead of qsort(void* _Base, size_t _NumOfElements, size_t _SizeOfElements, int (*_CompareFunction)(const void*, const void*) in <corecrt_search.h>. Also, since compare is useless, the _Pr can be a simple lambda [&](point& a, point& b){return ccwCheck(points[0],a,b) ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > + //selecting origin(the element with y minimum) + double yMin = points[0].y; + int min = 0; + for (size_t i = 1; i < points.size(); i++) { + double y = points[i].y; + if ((y < yMin) || (yMin == y && points[i].x < points[min].x)){ + yMin = points[i].y; + min = i; + } + } + swap(points[0], points[min]); + origin = points[0]; + + //sorting by polar angle and removing duplicates + qsort(&points[1], points.size() - 1, sizeof(point), compare); + std::vector<point> pointsSorted; You don't needed to create new one. original one is OK to edit ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > + int o = ccwCheck(origin, *p1, *p2); + if (o == 0) + return (distSq(origin, *p2) >= distSq(origin, *p1))? -1 : 1; + + return (o == 1)? -1: 1; +} + +void print(std::vector<point> points) +{ + std::cout << "the points of hull are as follows:\n"; + for (size_t i = 0; i < points.size(); i++) { + std::cout << "(" << points[i].x << "," << points[i].y << ")\n"; + } +} + +std::vector<point> grahamScan(std::vector<point> points){ I prefer to make this function more *c++y* by taking the begin and end iterator as argument instead of copying the input. ⬇️ Suggested change -std::vector<point> grahamScan(std::vector<point> points){ +template<typename Iter> +std::vector<point> grahamScan(Iter first, Iter last){ ------------------------------ In contents/graham_scan/code/c++/graham_scan.cpp <#616 (comment)> : > + origin = points[0]; + + //sorting by polar angle and removing duplicates + qsort(&points[1], points.size() - 1, sizeof(point), compare); + std::vector<point> pointsSorted; + pointsSorted.push_back(origin); + for(size_t i = 1; i < points.size(); i++){ + while(i < points.size() - 1 && ccwCheck(origin, points[i], + points[i+1]) == 0){ + i++; + } + pointsSorted.push_back(points[i]); + } + + //creating the convex hull + std::vector<point> hull; As mentioned, the algorithm shouldn't create a new vector to store hull. To save memory and expensive append() operations, we ultimately look for points that should be on the hull and swap them with the first elements in the array. If there are Melements on the hull, then the first M elements in our output random distribution of points will be the hull. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#616?email_source=notifications&email_token=AJTHDCXYZ4ZOZGWUVJSWJADP3ZDFVA5CNFSM4H2EKRJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4K7H6A#pullrequestreview-253096952>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTHDCTJOE3J4PPJ7ZUNNJLP3ZDFVANCNFSM4H2EKRJA> .

leios · 2019-07-06T10:40:23Z

Thanks for the review here @dovisutu!

Are you on discord? I apologize if you are there already and I missed you. A lot of discussion happens there.

dovisutu · 2019-07-06T13:04:07Z

Thanks for the review here @dovisutu!
Are you on discord? I apologize if you are there already and I missed you. A lot of discussion happens there.

I'd like to, but I can't login discord here in China...
Maybe try my VPN again...
(It worked, but 3 months ago it broke.)

…rithm-archive into graham_scan_in_cpp

dovisutu

Second round of reviewing, without testing on my maching.

dovisutu · 2019-07-07T08:20:08Z

contents/graham_scan/code/c++/graham_scan.cpp

+  std::vector<point>::const_iterator first = points.begin();
+  std::vector<point>::const_iterator last = points.begin()+m;
+  std::vector<point> hull(first, last);
+  return hull;


I prefer to inline these statements:

Suggested change

return hull;

return std::vector<point>(points.begin(), points.begin() + m);

dovisutu · 2019-07-07T08:23:07Z

contents/graham_scan/code/c++/graham_scan.cpp

+            }
+    }
+    m++;
+    std::swap<point>(points[i],points[m]);


You don't have to specify type. Automatic Type Inference will infers that _Ty is Point.
Also, spacing please.

Suggested change

std::swap<point>(points[i],points[m]);

std::swap(points[i], points[m]);

dovisutu · 2019-07-07T08:26:00Z

contents/graham_scan/code/c++/graham_scan.cpp

+
+void print(std::vector<point> points){
+  std::cout << "points in hull are as follows: \n";
+  for (size_t i = 0; i < points.size(); i++) {


I prefer to use for with range similar with for each in other languages. Like this:

Suggested change

for (size_t i = 0; i < points.size(); i++) {

for (auto p : points) {

Then use p instead of points[i] below.

dovisutu · 2019-07-07T08:29:00Z

contents/graham_scan/code/c++/graham_scan.cpp

@@ -0,0 +1,66 @@
+#include<iostream>


Please space between #include and header in order to be consistent with other implementations in AAA.

dovisutu · 2019-07-07T08:30:10Z

contents/graham_scan/code/c++/graham_scan.cpp

+      lowIndex = i;
+    }
+  }
+  std::swap<point>(points[0], points[lowIndex]);


You don't have to specify type. Automatic Type Inference will infers that _Ty is Point.

Suggested change

std::swap<point>(points[0], points[lowIndex]);

std::swap(points[0], points[lowIndex]);

dovisutu · 2019-07-07T08:31:02Z

contents/graham_scan/code/c++/graham_scan.cpp

+  point pivot = points[0];
+
+  //sorting points by polar angle
+  sort(points.begin()+1, points.end(), [pivot](point a, point b){


sort is in namespace std.

Suggested change

sort(points.begin()+1, points.end(), [pivot](point a, point b){

std::sort(points.begin()+1, points.end(), [pivot](point a, point b){

dovisutu · 2019-07-07T08:33:06Z

contents/graham_scan/code/c++/graham_scan.cpp

+  std::vector<point> points = {{-5, 2}, {5, 7}, {-6, -12}, {-14, -14}, {9, 9},
+                              {-1, -1}, {-10, 11}, {-6, 15}, {-6, -8}, {15, -9},
+                              {7, -7}, {-2, -9}, {6, -5}, {0, 14}, {2, 8}};
+  std::vector<point> hull = grahamScan(points);


I'd like a printing statment before finding hull, in order to show the points.

dovisutu · 2019-07-07T08:34:30Z

contents/graham_scan/code/c++/graham_scan.cpp

+
+struct point{
+  float x;
+  float y;


Why use float here? I think double is OK.

leios · 2019-07-07T09:10:23Z

@dovisutu No pressure on the discord, then! I just wanted to make sure you were kept in-the-loop with the latest changes, but most of that discussion is at least duplicated here on github.

Thanks for all the help!

dovisutu · 2019-07-07T11:03:47Z

@dovisutu No pressure on the discord, then! I just wanted to make sure you were kept in-the-loop with the latest changes, but most of that discussion is at least duplicated here on github.
Thanks for all the help!

BTW, I just fixed my VPN...
I'll try to join discord chat then.

AkashDhiman · 2019-07-07T17:31:34Z

I am getting a somewhat odd runtime error, the code compiles and the output is also shown but (only sometimes) with the error:

*** Error in `./a.out': free(): invalid next size (fast): 0x00005577d36da230 ***

after which it prints a memory map which I have not shown here.
I suspect it to be a buffer overflow maybe? Can anyone give me a suggestion on how to deal with it?

AkashDhiman · 2019-07-07T18:32:05Z

I think I fixed the runtime error, I believe that in the previous commit the runtime error was due to line 32 for loop, where the iterator could access location beyond the last element of vector.

dovisutu

There's another runtime error:

debug assertion failed!
Program: ......
File: ...\xutility
Line: 629

Expression: invalid comparer

This review fixes that.

dovisutu · 2019-07-08T01:50:23Z

contents/graham_scan/code/c++/graham_scan.cpp

+};
+
+bool ccw(point a, point b, point c){
+  return ((b.x - a.x)*(c.y - a.y) >= (b.y - a.y)*(c.x - a.x));


This is not a strict weak ordering Predication.

Suggested change

return ((b.x - a.x)*(c.y - a.y) >= (b.y - a.y)*(c.x - a.x));

return ((b.x - a.x)*(c.y - a.y) > (b.y - a.y)*(c.x - a.x));

dovisutu

At least to me, the code is fine now.

leios

I agree with @dovisutu, the code looks fine, so I am happy to merge it!

implemented graham scan in cpp

8875a08

c252 added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jun 21, 2019

dovisutu suggested changes Jun 22, 2019

View reviewed changes

AkashDhiman added 2 commits July 7, 2019 01:22

improved code for graham-scan implementation in cpp

248e5ae

Merge branch 'master' of https://github.com/algorithm-archivists/algo…

49b73d1

…rithm-archive into graham_scan_in_cpp

dovisutu suggested changes Jul 7, 2019

View reviewed changes

improved implementation based on suggestions

f3e18a9

fixed the runtimeerror

bedad3e

dovisutu suggested changes Jul 8, 2019

View reviewed changes

fixed runtime error based on suggestion

c4e859d

dovisutu approved these changes Jul 8, 2019

View reviewed changes

leios approved these changes Jul 10, 2019

View reviewed changes

leios merged commit e067fcb into algorithm-archivists:master Jul 10, 2019

	return ( crossProduct > 0)? -1 : 1;
	return (b.y - a.y) * (c.x - b.x) - (b.x - a.x) * (c.y - b.y);

	return hull;
	return std::vector<point>(points.begin(), points.begin() + m);

	std::swap<point>(points[i],points[m]);
	std::swap(points[i], points[m]);

	for (size_t i = 0; i < points.size(); i++) {
	for (auto p : points) {

	std::swap<point>(points[0], points[lowIndex]);
	std::swap(points[0], points[lowIndex]);

	sort(points.begin()+1, points.end(), [pivot](point a, point b){
	std::sort(points.begin()+1, points.end(), [pivot](point a, point b){

	return ((b.x - a.x)(c.y - a.y) >= (b.y - a.y)(c.x - a.x));
	return ((b.x - a.x)(c.y - a.y) > (b.y - a.y)(c.x - a.x));

Uh oh!

implemented graham scan in C plus plus #616

implemented graham scan in C plus plus #616

Uh oh!

Conversation

AkashDhiman commented Jun 21, 2019

Uh oh!

dovisutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkashDhiman commented Jun 22, 2019 via email

Uh oh!

leios commented Jul 6, 2019

Uh oh!

dovisutu commented Jul 6, 2019

Uh oh!

dovisutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leios commented Jul 7, 2019

Uh oh!

dovisutu commented Jul 7, 2019

Uh oh!

AkashDhiman commented Jul 7, 2019

Uh oh!

AkashDhiman commented Jul 7, 2019

Uh oh!

dovisutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dovisutu left a comment

Choose a reason for hiding this comment

Uh oh!

leios left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!