postgrespro / pgsphere

PgSphere provides spherical data types, functions, operators, and indexing for PostgreSQL.
https://pgsphere.org
BSD 3-Clause "New" or "Revised" License
16 stars 14 forks source link

Add spoly_is_convex #43

Closed ggnmstr closed 1 year ago

ggnmstr commented 1 year ago

Adds function to determine whether the spoly is convex. Algorithm based on https://mathoverflow.net/questions/193569/determining-orientation-of-spherical-polygons

vitcpp commented 1 year ago

@ggnmstr thank you for the PR. I think everything is ok except of some trivial issues. The tests should be fixed - could you please try to run tests without healpix:

I would also ask to start the commit comment from capital lettet as it was done for other commits.

dura0ok commented 1 year ago

fix_tests.patch Please @ggnmstr rename it to .patch extension and apply it! This fix your tests!!

ggnmstr commented 1 year ago

@stepan-neretin7 Thank you, it works, seems like there's a problem with healpix on my computer.

dura0ok commented 1 year ago

@ggnmstr please, start commit with capital letter

esabol commented 1 year ago

@stepan-neretin7 wrote:

@ggnmstr please, start commit with capital letter

Do you mean the title of the pull request, @stepan-neretin7 ? The one commit message at https://github.com/postgrespro/pgsphere/pull/43/commits does start with a capital letter.

dura0ok commented 1 year ago

@stepan-neretin7 wrote:

@ggnmstr please, start commit with capital letter

Do you mean the title of the pull request, @stepan-neretin7 ? The one commit message at https://github.com/postgrespro/pgsphere/pull/43/commits does start with a capital letter.

Yes, I think it's a good idea. I also suggest that you also specify the issue number in the comment, so that he automatically linked it to the desired issue even before the merge.

vitcpp commented 1 year ago

@ggnmstr There are some conflicts. Could you please do rebase on the newest version and fix the conflicts?

esabol commented 1 year ago

@ggnmstr: CI failures. Please check and fix "make test" results.

https://app.travis-ci.com/github/postgrespro/pgsphere/jobs/607409643

vitcpp commented 1 year ago

To finalize PR the following changes are required:

dura0ok commented 1 year ago

SELECT spoly_is_convex(NULL); spoly_is_convex

(1 row)

why you return null? is this correct for interface?

ggnmstr commented 1 year ago

@stepan-neretin7, this "return null" behavior is common among all functions, I don't think it is possible to change something just in place.

dura0ok commented 1 year ago

@stepan-neretin7, this "return null" behavior is common among all functions, I don't think it is possible to change something just in place.

but in your code you return false.. Very strange for me..

vitcpp commented 1 year ago

SELECT spoly_is_convex(NULL);

spoly_is_convex (1 row)

why you return null? is this correct for interface?

I agree, it should return false. The third value is unnecessary. To fix it STRICT keyword should be removed in CREATE FUNCTION command. I also propose to update the tests by adding this case.

ggnmstr commented 1 year ago

@vitcpp. I fixed it and added a test, thanks for the information.

dura0ok commented 1 year ago

for (int i = 0; i < poly->npts; i++) please move the variable initialization to the top of the function @ggnmstr

vitcpp commented 1 year ago

There are some conflicts that should be resolved. @ggnmstr Could you please rebase your branch to the latest postgrespro/pgsphere version and fix conflicts?

ggnmstr commented 1 year ago

spherepoly_is_convex() functions seems to be ok as the first working version, but the code is not optimized. There are duplicate calculations in the for loop that can slowdown the performance.

Yes, I agree with that. I think I'll optimize it a bit later.

vitcpp commented 1 year ago

@ggnmstr Thank you for your contribution. I plan to merge PR #40 first, your PR will be the next. I guess, new differences may appear - one more iteration may be required to fix the tests. It seems to be a problem of init_test that should be redesigned in the future.

vitcpp commented 1 year ago

@ggnmstr Could you please do one more iteration - re-sync your branch with this repo, resolve conflicts and force-push your changes? Your PR will be the next. Thank you in advance!

ggnmstr commented 1 year ago

@vitcpp, Ok, I did it. The tests are OK, I guess, I just realized that I was typing "HEAPLIX" all the time and then wondering why is it not working on my PC

vitcpp commented 1 year ago

@ggnmstr Thank you very much!