- Notifications
You must be signed in to change notification settings - Fork15
Added calculation of the distance between a line and a point#40
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
esabol commentedAug 2, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Darya177777 : CI tests failed. Make sure your fork is sync'ed with this repo. You might need to just update the line numbers in some expected output files? |
Darya177777 commentedAug 4, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for this! Looks pretty good to me. Just some minor changes, I think.
doc/operators.sgm Outdated
| @@ -331,7 +331,7 @@ | |||
| a non-boolean operator returning the distance between two | |||
| objects in radians. Currently, | |||
| <application>pgSphere</application> supports only distances | |||
| between points, circles, and between point andcircle. If the | |||
| between points, circles,between pointandcircle, andbetween point andline. If the | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this line exceed 79 characters? If so, please word-wrap.
pgs_line.sql.in Outdated
| IMMUTABLE STRICT; | ||
| COMMENT ON FUNCTION dist(sline, spoint) IS | ||
| 'distance between spherical line and spherical point'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All the other COMMENT ON FUNCTIONs in this file start with "returns..." I know that seems redundant, but I think it's best to fit in with the convention of the other COMMENTs.
expected/line.out Outdated
| @@ -134,4 +134,34 @@ SELECT sline ( spoint '(0, 0d)', spoint '(0.000001d, 0d)' ) # | |||
| f | |||
| (1 row) | |||
| -- checking the distance between a line and a point | |||
| SELECT round( CAST(sline '( 90d, 0d, 0d, XYZ ), 40d ' <-> spoint '( 0d, 90d )' as numeric), 14); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about the distance between a point and a line? I.e., the commutative property. Is that implemented? If not, it should be. If it is, then it should be tested.
SELECT round( CAST(spoint '( 0d, 90d )' <-> sline '( 90d, 0d, 0d, XYZ ), 40d ' as numeric), 14);All fixed. |
OK, looking better!@vitcpp will probably have some comments as well. |
@Darya177777 Could you also please rebase on the newest version and update the upgrade script upgrade_scripts/pg_sphere--1.2.3--1.3.0.sql.in. Such script is used when updating the extension to a newer version using ALTER EXTENSION UPDATE TO command. Once you have introduced a new function the upgrade script should be updated. |
src/line.c Outdated
| vector3d_spoint(&fp, &v_beg); | ||
| vector3d_spoint(&sp, &v_end); | ||
| if (FPle(spoint_dist(p, &fp), spoint_dist(p, &sp))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would propose to this code faster like this:
fpdist = spoint_dist(p, &fp);
spdist = spoint_dist(p, &sp);
return Min(fpdist, spdist);
src/line.c Outdated
| Datum | ||
| sphereline_point_distance(PG_FUNCTION_ARGS) | ||
| { | ||
| SLine *s = (SLine *) PG_GETARG_POINTER(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would propose to add const prefix in local variable declarations.
src/line.c Outdated
| Datum | ||
| sphereline_point_distance_com(PG_FUNCTION_ARGS) | ||
| { | ||
| SPoint *p = (SPoint *) PG_GETARG_POINTER(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would propose to add const prefix in local variable declarations.
expected/line.out Outdated
| 0.87266462599717 | ||
| (1 row) | ||
| SELECT round( CAST(sline '( 90d, 0d, 0d, XYZ ), 40d ' <-> spoint '( 0d, 90d )' as numeric), 14) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think round(CAST... is not necessary.
I would propose to add SET extra_float_digits TO -3; in the beginning of file. It reduces the number of output digits.
I've attached the modified line.sql
line.sql.zip
vitcpp commentedAug 7, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Darya177777 Please, ignore this comment. It seems I was wrong when claimed these cases to be wrong.
|
@Darya177777 Could you please squash two commits into one. |
doc/index.html Outdated
| @@ -0,0 +1,227 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This file is generated. It should not be added in the commit. Could you please remove it?
doc/types.html Outdated
| @@ -0,0 +1,272 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This file should be removed from the commit.
doc/x20.html Outdated
| @@ -0,0 +1,243 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove this file from the commit.
doc/x46.html Outdated
| @@ -0,0 +1,423 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove this file from the commit.
expected/init_test.out Outdated
| @@ -0,0 +1,37 @@ | |||
| SET client_min_messages TO NOTICE; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove this file from the commit.
log/initdb.log Outdated
| @@ -0,0 +1,37 @@ | |||
| Running in no-clean mode. Mistakes will not be cleaned up. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove this file from the commit.
log/postmaster.log Outdated
| @@ -0,0 +1,376 @@ | |||
| 2023-08-08 15:16:01.222 +07 postmaster[14615] LOG: starting PostgreSQL 15.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove this file from the commit.
logfile Outdated
| @@ -0,0 +1,749 @@ | |||
| 2023-08-08 14:58:03.794 +07 [12364] LOG: starting PostgreSQL 15.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove this file from the commit.
f44e926 tof62c70fComparePR seems to be ok. I need to do some testing before merge. |
@vitcpp, doesn't GitHub automatically squash commits on merge these days? I've been under the impression that it does, but maybe I'm used to GitLab. |
@esabol I'm not sure about github feature to squash commits. I will check it. I'm concerned which final comment will appear in commit after squashing. I'm not sure that simple joining of commit messages is a good idea. Furthermore, it is harder to review multiple commits where latest commit overwrites some changes of the previous commit. PR should contain the final version of the changes. It can contain multiple commits but these commits should be used to logically separate the changes. They should not touch the same code. |
@Darya177777 Could you please fix the tests? Thank you. |
src/sscan.c Outdated
| @@ -1,6 +1,6 @@ | |||
| #line 2 "sscan.c" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This file is generated. It should be removed from the commit.
d201938 to337928eCompare@Darya177777 You have added sscan.c file to the commit. It shouldn't be added. Could you please remove it from the commit. |
@vitcpp , All fixed |
@vitcpp, you can edit the merge commit message to whatever you want when squashing and merging, according to this:https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits |
@esabol Thank you very much! I will take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Darya177777 It seems we missed to make some changes in pg_sphere--1.2.3--1.3.0.sql.in. This script is used to upgrate pgsphere versions in the existing database. Just copy the contents of pgs_line.sql.in into this file.
@vitcpp I have made some changes in pg_sphere--1.2.3--1.3.0.sql.in. Please, re-review. |
There was a problem hiding this 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.
No description provided.