Opened 6 years ago

Closed 6 years ago

#3422 closed defect (fixed)

test_lwgeom_split fails on various architectures

Reported by: Bas Couwenberg Owned by: strk
Priority: medium Milestone: PostGIS 2.2.2
Component: postgis Version: 2.2.x
Keywords: Cc:

Description

The Debian package builds for postgis 2.2.1 failed on various architectures due to test failure of test_lwgeom_split introduced for #3401 in r14549:

Suite: split
  Test: test_lwline_split_by_point_to ...passed
  Test: test_lwgeom_split ...[cu_split.c:267]
 Expected: -20
 Obtained: -20
[cu_split.c:272]
 Expected: -20
 Obtained: -20
FAILED
    1. cu_split.c:267  - CU_ASSERT_EQUAL(pt.x,(double)-20)
    2. cu_split.c:272  - CU_ASSERT_EQUAL(pt.x,(double)-20)

Build logs: arm64, ppc64el, s390x

Not all build results are available yet, the others will become available via the buildd status page

Change History (22)

comment:1 by Bas Couwenberg, 6 years ago

The powerpc build also failed due to the same test failure.

amd64, i386 & mipsel are currently the only architectures on which the test succeeds.

comment:2 by robe, 6 years ago

I'm a bit puzzled why expected and obtained values look the same.

comment:3 by Bas Couwenberg, 6 years ago

I must admit that my knowledge of these ports is not sufficient to explain this test failure. I do have access to porterboxes for these architectures, so I'm happy to help troubleshoot this issue on the architectures in question.

comment:4 by gdt, 6 years ago

Are we sure the tests are looking for a close value as a double, vs exact bits, and that we aren't comparing the value obtained with extended precision doubles (which are more than IEEE754, common on x86) vs standard precision doubles (typical on everything that isn't x86 and isn't vax).

comment:6 by strk, 6 years ago

Sebastic: could you give me access to one of those machines to take a look ? It sounds like closest_point_on_segment is not returning the input point even when it is already _on_ the input segment.

comment:7 by strk, 6 years ago

It could also be tested that the following returns TRUE:

WITH inp AS ( SELECT 
 'LINESTRING(-180 0,0 0)'::geometry l, 
 'POINT(-20 0)'::geometry p 
) SELECT ST_Equals(ST_ClosestPoint(l,p), p) FROM inp;

comment:8 by Bas Couwenberg, 6 years ago

I cannot provide access to the Debian porterboxes, if you really need that access we can request a guest account.

Since that's quite an involved process, it's probably better for me to just run the tests. I'll do that after work tonight.

comment:9 by pramsey, 6 years ago

Resolution: fixed
Status: newclosed

Added tolerance to the double tests at r14589 in 2.2 and r14590 in trunk

comment:10 by Bas Couwenberg, 6 years ago

Resolution: fixed
Status: closedreopened

With the changes from r14589 added to the Debian package, regress/st_modedgesplit now fails:

--- regress/st_modedgesplit_expected    2016-01-11 19:02:03.825936435 +0000
+++ /tmp/pgis_reg/test_21_out   2016-01-11 19:06:18.937928368 +0000
@@ -47,4 +47,6 @@
 t3401.N2|2
 t3401.L1|1
 t3401.split|3
+t3401.valid_after|edge start node geometry mis-match|2|3
+t3401.valid_after|edge end node geometry mis-match|1|3
 t3401.end|Topology 'bug3401' dropped

comment:11 by pramsey, 6 years ago

You're certain? The changes in r14589 are completely contained in the library test module, the online tests you see failing are completely unrelated.

comment:12 by Bas Couwenberg, 6 years ago

Resolution: fixed
Status: reopenedclosed

Are you sure it's completely unrelated?

The test is for the same issue (#3401), I think the problem just got moved.

As I don't have time to endlessly debate this, I'll close this issue again because test_lwgeom_split doesn't fail any more. I'll keep the tests disabled for the problematic architectures.

comment:13 by strk, 6 years ago

Resolution: fixed
Status: closedreopened

The tests _are_ related. Robustness issues in lwgeom_split were found to be the cause for corrupting topology, so rather than hiding the failure in the C library, we should keep it exposed and try to fix it instead.

The correct solution would be to keep the split point as a vertex, rather than re-projecting it, when found to be already ON the segment.

comment:14 by pramsey, 6 years ago

Related insofar that you have to pass the C tests in order to even run the online tests. I see.

comment:15 by strk, 6 years ago

Owner: changed from pramsey to strk
Status: reopenednew

I've a temptative fix locally, about to push it.

comment:16 by strk, 6 years ago

(In [14594]) Avoid any drift of cutter point on lines split

Should fix splitting operations on at least arm64, ppc64el and s390x. See #3422 and #3401

Also turn ASSERT_DOUBLE_EQUAL back to a tolerance-free check (better use a different name for tolerance-aware check, so caller can decide)

comment:17 by strk, 6 years ago

Sebastic, could you please try testing the trunk branch, as of r14594 ?

comment:18 by strk, 6 years ago

(In [14595]) Avoid any drift of cutter point on lines split

Should fix splitting operations on at least arm64, ppc64el and s390x. See #3422 and #3401 (for 2.2 branch)

Also turn ASSERT_DOUBLE_EQUAL back to a tolerance-free check (better use a different name for tolerance-aware check, so caller can decide)

comment:19 by strk, 6 years ago

Actually, also backported to 2.2 branch with r14595

comment:20 by Bas Couwenberg, 6 years ago

The root disk in my workstation is dying with bad sectors, so I have to fix that first to be fully functional again. I can probably test the changes from r14595 this weekend.

comment:21 by Bas Couwenberg, 6 years ago

With the changes from r14595 the tests succeed on arm64, powerpc, ppc64el, s390x.

Looks like this is fixed properly now, thanks!

comment:22 by strk, 6 years ago

Resolution: fixed
Status: newclosed

thanks for testing

Note: See TracTickets for help on using tickets.