NotificationsYou must be signed in to change notification settings
Fork15
Star22

Add spoly_is_convex#43

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

vitcpp merged 1 commit intopostgrespro:masterfromggnmstr:poly_convex

Aug 12, 2023

Merged

Add spoly_is_convex#43

vitcpp merged 1 commit intopostgrespro:masterfromggnmstr:poly_convex

Aug 12, 2023

Conversation

Copy link

Contributor

ggnmstr commentedAug 2, 2023

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

ggnmstr force-pushed thepoly_convex branch 2 times, most recently fromd89edcf toa0be9d3Compare

August 3, 2023 09:40

vitcpp requested changes

Aug 3, 2023

View reviewed changes

Copy link

Contributor

vitcpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

In general everything is ok except of some trivial issues.

src/polygon.c OutdatedShow resolvedHide resolved

Copy link

Contributor

vitcpp commentedAug 3, 2023•
edited
Loading

@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:

gmake USE_HEALPIX=0 install
gmake USE_HEALPIX=0 test

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

ggnmstr force-pushed thepoly_convex branch 3 times, most recently fromb61c748 to5d264e6Compare

August 4, 2023 03:59

Copy link

dura0ok commentedAug 4, 2023•
edited
Loading

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

ggnmstr force-pushed thepoly_convex branch from5d264e6 toc3b3cf7Compare

August 4, 2023 05:34

Copy link

ContributorAuthor

ggnmstr commentedAug 4, 2023

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

ggnmstr requested a review fromvitcpp

August 4, 2023 05:49

Copy link

dura0ok commentedAug 4, 2023

@ggnmstr please, start commit with capital letter

Copy link

Contributor

esabol commentedAug 4, 2023

@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 athttps://github.com/postgrespro/pgsphere/pull/43/commits does start with a capital letter.

Copy link

dura0ok commentedAug 4, 2023

@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 athttps://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.

ggnmstr changed the title~~add spoly_is_convex~~Add spoly_is_convex

Aug 5, 2023

Copy link

Contributor

vitcpp commentedAug 5, 2023

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

Copy link

Contributor

esabol commentedAug 5, 2023•
edited
Loading

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

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

ggnmstr force-pushed thepoly_convex branch 3 times, most recently from544f4f6 to4570340Compare

August 6, 2023 04:32

esabol suggested changes

Aug 6, 2023

View reviewed changes

expected/init_test.out.in Outdated

		psql:pg_sphere.test.sql:8594: NOTICE: argument type pointkey is only a shell
		psql:pg_sphere.test.sql:8600: NOTICE: argument type pointkey is only a shell
		psql:pg_sphere.test.sql:9167: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9173: NOTICE: argument type smoc is only a shell

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Add linefeed to the end of this file.

expected/init_test_healpix.out.in Outdated

		psql:pg_sphere.test.sql:9181: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9187: NOTICE: argument type smoc is only a shell
		psql:pg_sphere.test.sql:9167: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9173: NOTICE: argument type smoc is only a shell

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Add a linefeed to the end of this file.

ggnmstr force-pushed thepoly_convex branch 5 times, most recently from2620fa7 toa35e842Compare

August 6, 2023 07:22

esabol suggested changes

Aug 6, 2023

View reviewed changes

expected/init_test_healpix.out.in Outdated

		psql:pg_sphere.test.sql:9181: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9187: NOTICE: argument type smoc is only a shell
		psql:pg_sphere.test.sql:9167: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9173: NOTICE: argument type smoc is only a shell

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You are almost there! Change "9167" to "9195" on line 1. Change "9173" to "9201" on line 2.

ggnmstr force-pushed thepoly_convex branch froma35e842 to5012568Compare

August 6, 2023 15:51

This comment was marked as outdated.

Copy link

Contributor

vitcpp commentedAug 7, 2023

To finalize PR the following changes are required:

Update pg_sphere--1.2.3--1.3.0.sql. Add command to create the implemented function.
Add new function description into Functions -> spoly functions section of the User Manual (gmake pdf).

Copy link

ContributorAuthor

ggnmstr commentedAug 8, 2023

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

Copy link

dura0ok commentedAug 8, 2023

@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..

esabol suggested changes

Aug 8, 2023

View reviewed changes

Copy link

Contributor

esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A couple more minor tweaks, please,@ggnmstr. Thank you!

src/polygon.c Outdated

		wsv,
		crs;
		float8cur = 0,
		prev = 0;

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do the variables on lines 1487-1493 line up vertically for you? They don't line up vertically in the diff output here on GitHub.com.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, they do.

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The default tab size on github is 8. It can be changes to 4 in Profile -> Settings -> Appearance -> Tab size... Not sure, how to change it for a specific repository and specific file type.

I changed it. For me it looks like this:

src/polygon.h Outdated

		/*
		* Checks whether a polygon is convex
		*/
		Datum spherepoly_is_convex(PG_FUNCTION_ARGS);

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Add spaces afterDatum so thatspherepoly_is_convex lines up vertically with the names of the functions above this.

doc/functions.sgm Outdated

		Spoly is convex
		</title>
		<para>
		Returns true if the specified spherical polygon is convex. Returns false otherwise.

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is this line longer than 79 characters? I can't tell looking at it in the web browser here. If it is, please word-wrap it.

ggnmstr force-pushed thepoly_convex branch 3 times, most recently fromff10fe4 toe740d2cCompare

August 8, 2023 06:04

Copy link

Contributor

vitcpp commentedAug 8, 2023

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 force-pushed thepoly_convex branch frome740d2c to6241f5aCompare

August 8, 2023 09:44

Copy link

ContributorAuthor

ggnmstr commentedAug 8, 2023

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

Copy link

dura0ok commentedAug 8, 2023•
edited
Loading

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

ggnmstr force-pushed thepoly_convex branch from6241f5a tob11fea7Compare

August 8, 2023 14:21

vitcpp approved these changes

Aug 9, 2023

View reviewed changes

Copy link

Contributor

vitcpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

vitcpp commentedAug 9, 2023

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Remove the line

ggnmstr force-pushed thepoly_convex branch from5725967 to41c0326Compare

August 10, 2023 13:08

esabol suggested changes

Aug 10, 2023

View reviewed changes

Copy link

Contributor

esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Still need to getmake test working.

expected/init_test.out.in Outdated

		@@ -33,3 +33,4 @@ psql:pg_sphere.test.sql:8622: NOTICE: argument type pointkey is only a shell
		psql:pg_sphere.test.sql:8628: NOTICE: argument type pointkey is only a shell
		psql:pg_sphere.test.sql:8634: NOTICE: argument type pointkey is only a shell
		psql:pg_sphere.test.sql:8640: NOTICE: argument type pointkey is only a shell

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why did you add this blank line? I think it should be removed.

Take a look at the diff output at the bottom of the log here:
https://app.travis-ci.com/github/postgrespro/pgsphere/jobs/607714835

You need to change the line numbers in this file to match the results so that there are no differences.

expected/init_test_healpix.out.in Outdated

		@@ -1,2 +1,3 @@
		psql:pg_sphere.test.sql:9207: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9213: NOTICE: argument type smoc is only a shell

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Remove this blank line. In this file, change "9207" to "9221" and change "9213" to "9227".

ggnmstr force-pushed thepoly_convex branch 2 times, most recently from006ad50 toe642169Compare

August 11, 2023 15:35

esabol approved these changes

Aug 11, 2023

View reviewed changes

Copy link

Contributor

esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me! Thank you for your contribution and perseverance,@ggnmstr!

vitcpp approved these changes

Aug 11, 2023

View reviewed changes

Copy link

Contributor

@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.

Copy link

Contributor

vitcpp commentedAug 11, 2023

@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!

Add spoly_is_convex

d8470f8

ggnmstr force-pushed thepoly_convex branch frome642169 tod8470f8Compare

August 12, 2023 04:07

Copy link

ContributorAuthor

ggnmstr commentedAug 12, 2023•
edited
Loading

@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

Copy link

Contributor

vitcpp commentedAug 12, 2023

@ggnmstr Thank you very much!

esabol approved these changes

Aug 12, 2023

View reviewed changes

vitcpp merged commit6720f3a intopostgrespro:master

Aug 12, 2023

Labels

None yet

4 participants

		@@ -28,6 +28,7 @@ COMMENT ON FUNCTION spoly_deg(float8[]) IS
		refer to the same occurrence and cover its
		latitude and longitude, respectively.';

		<<<<<<< HEAD

Movatterモバイル変換

Add spoly_is_convex#43

Add spoly_is_convex#43

Uh oh!

Conversation

ggnmstr commentedAug 2, 2023

Uh oh!

vitcpp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vitcpp commentedAug 3, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

dura0ok commentedAug 4, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ggnmstr commentedAug 4, 2023

Uh oh!

dura0ok commentedAug 4, 2023

Uh oh!

esabol commentedAug 4, 2023

Uh oh!

dura0ok commentedAug 4, 2023

Uh oh!

vitcpp commentedAug 5, 2023

Uh oh!

esabol commentedAug 5, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

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!

This comment was marked as outdated.

Uh oh!

vitcpp commentedAug 7, 2023

Uh oh!

ggnmstr commentedAug 8, 2023

Uh oh!

dura0ok commentedAug 8, 2023

Uh oh!

esabol 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!

vitcpp commentedAug 8, 2023

SELECT spoly_is_convex(NULL);

Uh oh!

ggnmstr commentedAug 8, 2023

Uh oh!

dura0ok commentedAug 8, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vitcpp left a comment

Choose a reason for hiding this comment

Uh oh!

vitcpp commentedAug 9, 2023

Uh oh!

ggnmstr commentedAug 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esabol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vitcpp commentedAug 3, 2023•
edited
Loading

dura0ok commentedAug 4, 2023•
edited
Loading

esabol commentedAug 5, 2023•
edited
Loading

dura0ok commentedAug 8, 2023•
edited
Loading

ggnmstr commentedAug 12, 2023•
edited
Loading