Add gravity center function #29

dura0ok · 2023-07-13T05:18:53Z

No description provided.

esabol · 2023-07-13T05:29:17Z

src/point.c

+	spoint_check(p);
+
+	PG_RETURN_POINTER(p);
+}


An EOL character is needed at the end of this file.

esabol · 2023-07-13T05:35:25Z

src/point.c

+
+	for (int i = 0; i < num_elements; i++) {
+		current_point = array_data[i];
+		//elog(LOG, "%lf\n", current_point.lat);


I would remove this line of code that has been commented out.

I would also propose to move 'int i' declaration to the function beginning. Standard C doesn't allow to declare types such the way.

esabol · 2023-07-13T05:37:38Z

src/point.c

+		elog(LOG, "%lf\n", v.x);
+		res.x+=v.x;
+		res.y+=v.y;
+		res.z+=v.z;


Add a space on both sides of the += operator on lines 289, 290, and 291.

Also, I think res.x, res.y, and res.z need to be initialized to zeroes before the for loop? This might explain the test failures seen on some PostgreSQL versions.

What happens if num_elements is zero, i.e. if the ARRAY passed to the function is empty? I think it should give an error or return NULL perhaps?

I think that the gravity center point (centroid) should be on the sphere in 2D space because pgsphere is about 2D space on the sphere. I'm not sure that the average point in 3d space will be on the sphere. May be to to average long/lat coordinates.

esabol · 2023-07-13T05:38:12Z

src/point.h

+/*
+ * Return gravity center from array of spherical points
+ */
+Datum get_gravity_center(PG_FUNCTION_ARGS);


The whitespace after Datum probably should be the same as on lines 91 and 86.

esabol · 2023-07-13T05:41:44Z

pgs_point.sql.in

@@ -167,3 +167,8 @@ CREATE OPERATOR  <-> (
 COMMENT ON OPERATOR <-> (spoint, spoint) IS
  'distance between spherical points';

+CREATE FUNCTION get_gravity_center(dots_vector SPoint[])


My initial reaction is that the function should be called gravity_center instead of get_gravity_center. I could be wrong about that though. Is there a convention or precedent for how functions should be named?

I propose to name it 'centroid'. For example, BOOST.Geometry names such function as centroid.

esabol · 2023-07-13T06:05:44Z

This constitutes an API change, so the version number will need to be incremented in some fashion. I'm not sure if that's been decided in issue #9 yet. An upgrade script will also be needed.

make test failed. You'll need to update expected/init_test.out.in at least.

make installcheck also failed with most of the tested PostgreSQL versions but not version 10. That's weird. Not sure why that is.

diff -U3 /home/travis/build/postgrespro/pgsphere/expected/points.out /home/travis/build/postgrespro/pgsphere/results/points.out
--- /home/travis/build/postgrespro/pgsphere/expected/points.out	2023-07-13 05:23:20.880316866 +0000
+++ /home/travis/build/postgrespro/pgsphere/results/points.out	2023-07-13 05:24:27.069036698 +0000
@@ -393,7 +393,7 @@
 ]);
     get_gravity_center    
 --------------------------
- (3.0436698 , 0.85893807)
+ (3.0436698 , -1.5707963)
 (1 row)

esabol · 2023-07-13T06:21:16Z

I'd like to see additional tests added: a test with only one spoint in the passed ARRAY and a test with zero spoints in the passed ARRAY.

vitcpp · 2023-07-13T06:38:27Z

I would propose to name it as 'centroid'. It is more laconic name. Boost geometry names such function as centroid.

Furthermore, we may overload such function for other geometry types to make it complete.

I would also propose to put test SQL for this functionality into a new file like centroid.sql, for example. Such a file separation is reasonable, because all the stuff related to this function will be placed in the single file. If we add some other overloaded functions like centroid(spoly) we will still have the single test file. The test code will not be placed into multiple files.

vitcpp · 2023-07-13T08:33:56Z

I also would consider the case when two points are located in the same big circle in opposite directions. The 3D center mass in this case reside in the center of the sphere. There are two points may be returned. Which point to choose?

* some little fixes * some little fixes * fix tests * tests * tests * reset * .. * .. * fix tests * fix tests * curr * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * logs * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * tryings * try to make unit test * current state * tryings * tryings * current state * current state * current state * current state * tryings

vitcpp · 2023-07-19T12:14:01Z

doc/constructors.sgm

@@ -86,7 +86,7 @@
              the <literal>y</literal>-axis.
            </simpara>
            <programlisting>
-<![CDATA[sql> SELECT strans ( 20.0*pi()/180.0, -270.0*pi()/180.0, 70.5*pi()/180.0, 'XZY');]]>


pi() call was removed from the formula. I guess it is a typo in the doc.

vitcpp · 2023-07-19T12:22:04Z

Closed until it will be polished in the author's original branch.

esabol suggested changes Jul 13, 2023

View reviewed changes

add base logic + 1 test

405e318

dura0ok force-pushed the master branch from 042f177 to 405e318 Compare July 13, 2023 06:17

Stepan added 3 commits July 19, 2023 11:08

try to fix tests

3c2f6a8

try to fix tests

1798f6c

vitcpp reviewed Jul 19, 2023

View reviewed changes

vitcpp closed this Jul 19, 2023

Add gravity center function #29

Add gravity center function #29

Uh oh!

Conversation

dura0ok commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitcpp Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vitcpp Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esabol commented Jul 13, 2023

Uh oh!

esabol commented Jul 13, 2023

Uh oh!

vitcpp commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitcpp commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitcpp commented Jul 19, 2023

Uh oh!

Uh oh!

vitcpp Jul 13, 2023 •

edited

Loading

vitcpp Jul 13, 2023 •

edited

Loading

vitcpp commented Jul 13, 2023 •

edited

Loading