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 Changed 6 years ago by Bas Couwenberg

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 Changed 6 years ago by robe

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

comment:3 Changed 6 years ago by Bas Couwenberg

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 Changed 6 years ago by gdt

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 Changed 6 years ago by strk

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 Changed 6 years ago by strk

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 Changed 6 years ago by Bas Couwenberg

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 Changed 6 years ago by pramsey

Resolution: fixed
Status: newclosed

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

comment:10 Changed 6 years ago by Bas Couwenberg

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 Changed 6 years ago by pramsey

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 Changed 6 years ago by Bas Couwenberg

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 Changed 6 years ago by strk

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 Changed 6 years ago by pramsey

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

comment:15 Changed 6 years ago by strk

Owner: changed from pramsey to strk
Status: reopenednew

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

comment:16 Changed 6 years ago by strk

(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 Changed 6 years ago by strk

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

comment:18 Changed 6 years ago by strk

(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 Changed 6 years ago by strk

Actually, also backported to 2.2 branch with r14595

comment:20 Changed 6 years ago by Bas Couwenberg

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 Changed 6 years ago by Bas Couwenberg

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

Looks like this is fixed properly now, thanks!

comment:22 Changed 6 years ago by strk

Resolution: fixed
Status: newclosed

thanks for testing

Note: See TracTickets for help on using tickets.