Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds ST_3DLineInterpolatePoint function #346

Closed
wants to merge 10 commits into from

Conversation

@troopa81 troopa81 referenced this pull request Nov 30, 2018
@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

@troopa81 Travis failed.

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Travis reports a memory leak:

Elapsed time =    6.881 seconds
==6375== 
==6375== HEAP SUMMARY:
==6375==     in use at exit: 7,937 bytes in 200 blocks
==6375==   total heap usage: 126,822 allocs, 126,622 frees, 6,893,071 bytes allocated
==6375== 
==6375== 80 (32 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 198 of 200
==6375==    at 0x483577F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6375==    by 0x4867BF5: lwline_construct_empty (lwline.c:66)
==6375==    by 0x113FA6: test_lwline_interpolate_point_3d (cu_algorithm.c:553)
==6375==    by 0x492D336: ??? (in /usr/lib/x86_64-linux-gnu/libcunit.so.1.0.1)
==6375==    by 0x492D56C: ??? (in /usr/lib/x86_64-linux-gnu/libcunit.so.1.0.1)
==6375==    by 0x492D9DD: CU_run_all_tests (in /usr/lib/x86_64-linux-gnu/libcunit.so.1.0.1)
==6375==    by 0x110432: main (cu_tester.c:177)
==6375== 
==6375== LEAK SUMMARY:
==6375==    definitely lost: 32 bytes in 1 blocks
==6375==    indirectly lost: 48 bytes in 2 blocks
==6375==      possibly lost: 0 bytes in 0 blocks
==6375==    still reachable: 7,857 bytes in 197 blocks
==6375==         suppressed: 0 bytes in 0 blocks
==6375== Reachable blocks (those to which a pointer was found) are not shown.
==6375== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==6375== 
==6375== For counts of detected and suppressed errors, rerun with: -v
==6375== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
make[2]: *** [Makefile:92: check] Error 1
make[2]: Leaving directory '/src/postgis/liblwgeom/cunit'
make[1]: *** [Makefile:204: check] Error 2
make[1]: Leaving directory '/src/postgis/liblwgeom'
make: *** [GNUmakefile:20: check] Error 1
[logbt] saw 'make' exit with code:2 (INT)

@Komzpa Komzpa requested a review from dbaston Dec 3, 2018

@Komzpa

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

List of things to polish before or during merge:

  • figure out whether vincent.mora@olandia.com has a typo in it, move to file header;
  • tests for ratio=1 and ratio=N on non-first segment of line, coverage suggests they are not tested;
  • link ST_LineInterpolatePoint to new function in docs;
  • rename postgis/ LWGEOM_line_interpolate_point_3d to ST_3DLineInterpolatePoint to match SQL name;
  • git-clang-format.

Extras:

  • figure out why function is structurally different form 2D, 2D version accepts a boolean flag and may return several points - where is it used, shall it be deleted too? Maybe ticket that or add comment why the difference.
@troopa81

This comment has been minimized.

Copy link
Author

commented Dec 3, 2018

What the typo problem exactly? I try to format email reference like it was done in other parts of the code, maybe it corrects.

Regarding the coverage, contrary to what report said, I just add some printf and the program reaches appropriate line of code when line is empty, when ratio=1 and when ratio=0.8. So I'm a bit confused, is it possible that there is a problem with coverage report?

3 following points are now OK.

Extras :
The structure is a bit different because we only need ST_3DLineInterpolatePoint instead of ST_LineInterpolatePoint, and I don't know is any one need ST_3DLineInterpolatePoints. I can create to a ticket in order to track the potential need.

/* If distance is one of the two extremes, return the point on that
* end rather than doing any expensive computations
*/
if ( distance == 0.0 || distance == 1.0 )

This comment has been minimized.

Copy link
@Algunenano

Algunenano Dec 3, 2018

Member

I'd suggest using FP_EQUALS instead of comparing doubles directly.

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

this is special shortcut case where == is needed, on other (even close) values we'd better go via full interpolation.

This comment has been minimized.

Copy link
@Algunenano

Algunenano Dec 3, 2018

Member

Is it? What about -0.0? Or 0.9999999999999983 which is, according to our internal standards, 1.0?

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

okay, then we need to change this function and the function right above it to match. :)

@Komzpa

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

@troopa81 e-mail: olandia.com is non existent.

Coverage: lwline.c lines 624, 635, 667-635 are not exercised by tests. Lines 672-673 look suspicious, as I don't quite see how can execution go there and what exactly are those float rounding errors.

formatting: line 673 has a double space, 624 is tabulated via spaces in lwline.c. clang-format by default only fixes your last commit, you should give it your base branch name to fix all the things: git-clang-format svn-trunk

troopa81 added 2 commits Dec 3, 2018

@Komzpa Komzpa self-requested a review Dec 3, 2018

@Komzpa
Komzpa approved these changes Dec 3, 2018

/***********************************************************************
* Interpolate a point along a line
* 3D version by vincent.mora@olandia.com

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

is it really olandia, or oslandia?

should probably go to the header

This comment has been minimized.

Copy link
@troopa81

troopa81 Dec 3, 2018

Author

My mistake, It's oslandia. It's corrected since commit 35b628f.

Where do you mean "in the header". This methods is not listed in the lwgeom_functions_analytic.h

<xref linkend="ST_AsText" />,
<xref linkend="ST_AsEWKT" />,
<xref linkend="ST_Length" />,
<xref linkend="ST_LineInterpolatePoint" />

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

would be good to link to ST_3DLineInterpolatePoint from ST_LineInterpolatePoint

This comment has been minimized.

Copy link
@troopa81

troopa81 Dec 3, 2018

Author

Done in commit dc2ab7c

<refsection>
<title>Examples</title>

<programlisting>--Return point 20% along 3d line

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

3D
can also be moved into a <para> just before <programlisting>

This comment has been minimized.

Copy link
@troopa81
@@ -545,7 +545,7 @@ static void test_lwline_interpolate_points(void)

static void test_lwline_interpolate_point_3d(void)
{
LWLINE* line;
LWLINE *line;

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

that's just latest commit formatted, for full one

git-clang-format svn-trunk
  • it will go over full diff of current branch to svn-trunk branch.

This comment has been minimized.

Copy link
@troopa81

troopa81 Dec 3, 2018

Author

Done in commit
5db1079

/* If distance is one of the two extremes, return the point on that
* end rather than doing any expensive computations
*/
if ( distance == 0.0 || distance == 1.0 )

This comment has been minimized.

Copy link
@Komzpa

Komzpa Dec 3, 2018

Member

this is special shortcut case where == is needed, on other (even close) values we'd better go via full interpolation.

@troopa81

This comment has been minimized.

Copy link
Author

commented Dec 3, 2018

Coverage: lwline.c lines 624, 635, 667-635 are not exercised by tests. Lines 672-673 look suspicious, as I don't quite see how can execution go there and what exactly are those float rounding errors.

I add some printf to on line 624, 635, 667 and even if the text is printed, the gcov file point out that there are not :

Output

 Test: test_lwline_interpolate_point_3d ...titi
test0
titi
test1
titi
test2
titi
test4
titi
test4
passed

lwline.c.gcov

        -:  624:	/* Empty.InterpolatePoint == Point Empty */
        4:  625:	if (lwline_is_empty(line))
call    0 returned 100%
branch  1 taken 0% (fallthrough)
branch  2 taken 100%
        -:  626:	{
    #####:  627:		printf("test0\n");
call    0 never executed
    #####:  628:		return lwpoint_construct_empty(line->srid, has_z, has_m);
call    0 never executed
        -:  629:	}

Here is the modified lwline file, the log file, and the gcov file
lwline.c.txt
log.txt
lwline.c.gcov.txt

I don't understand what I am missing!

The last lines are not covered indeed. They cover the case where the distance computation (length+slength) with be slighly (1.00000000001) greater than 1 because of floating point rounding error.

@strk strk closed this in 8bc9492 Dec 3, 2018

@Komzpa

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

@troopa81 thank you for getting this polished!

I merged the PR.

Something is fishy in the coverage, will keep that in mind in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.