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

Fix broken median tests #205

Closed
wants to merge 3 commits into
base: svn-trunk
from

Conversation

Projects
None yet
2 participants
@Algunenano
Member

Algunenano commented Feb 5, 2018

Changed the method to check the tolerance in the test to use the sum of the distances to the obtained point instead of comparing directly coordinates.

Also I had some issues with GCC and -O3 as it introduces some errors that causes the condition (intermediate point included in the set) to be missed. It works fine with -O0 or -O2 and clang -O3.

@Algunenano

This comment has been minimized.

Member

Algunenano commented Feb 5, 2018

@codecov

This comment has been minimized.

codecov bot commented Feb 5, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (svn-trunk@d613598). Click here to learn what that means.
The diff coverage is 96.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             svn-trunk     #205   +/-   ##
============================================
  Coverage             ?   79.31%           
============================================
  Files                ?      201           
  Lines                ?    62896           
  Branches             ?        0           
============================================
  Hits                 ?    49886           
  Misses               ?    13010           
  Partials             ?        0
Impacted Files Coverage Δ
liblwgeom/lwgeom_median.c 97.34% <100%> (ø)
liblwgeom/cunit/cu_algorithm.c 98.89% <90.9%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d613598...64b4bca. Read the comment docs.

@Algunenano

This comment has been minimized.

Member

Algunenano commented Feb 5, 2018

After further investigation it seems that the test relies in 80bit precision to work correctly. I'm working to find a test that works with just 64 bit floats.

@Algunenano

This comment has been minimized.

Member

Algunenano commented Feb 5, 2018

It seems than when looking for fine grained results certain issues dependent on how many bits are used for the float operations start to appear.

I've added (hidden behind #ifdef 0) a test that fails when using 80-bit precision and another one that fails under 64bit doubles.
Also I've added a test that fails under both (infinite loop). I think it should have been solved by #176 but isn't so a deeper look is needed (or maybe switching to a different algorithm), but I'd rather have the build bots green now and deal with this if a different issue.

@pramsey

This comment has been minimized.

Member

pramsey commented Feb 7, 2018

I'm seeing regression failures in median now on OSX.

Butterfly:~/Code/postgis-svn/liblwgeom/cunit pramsey$ ./cu_tester test_median_robustness

Running test 'test_median_robustness' in suite 'algorithm'.
median_test input MULTIPOINT ((1 -1 3), (1 0 2), (2 1 1)) (parsed MULTIPOINT(1 -1 3,1 0 2,2 1 1)) expected POINT(1 0 2) got POINT(1 0 2)
median_test input MULTIPOINT ZM ((0 0 20000 0.5),(0 0 59000 0.5),(0 48000 -20000 1.3),(0 -48000 -20000 1.3),(0 -3000 -3472.22222222222262644208967685699462890625 1),(0 3000 3472.22222222222262644208967685699462890625 1),(0 0 -1644.736842105263121993630193173885345458984375 1),(0 0 1644.736842105263121993630193173885345458984375 1)) (parsed MULTIPOINT(0 0 20000 0.5,0 0 59000 0.5,0 48000 -20000 1.3,0 -48000 -20000 1.3,0 -3000 -3472.22222222 1,0 3000 3472.22222222 1,0 0 -1644.73684211 1,0 0 1644.73684211 1)) expected POINT(0 0 0) got POINT(0 0 0)

    FAILED - asserts -  25 passed,   2 failed,  27 total.


  1. cu_algorithm.c:1171  - CU_ASSERT_TRUE(passed)
  2. cu_algorithm.c:1171  - CU_ASSERT_TRUE(passed)
@Algunenano

This comment has been minimized.

Member

Algunenano commented Feb 7, 2018

Have you enabled the test or are you testing something different from this PR?

I disabled it (and added another one disabled too) because they fail depending on the precision of your double operations (64 or 80 bit).
I'm not sure if the failures are due to rounding or due to the modification to the algorithm (when finding a point in the input) breaking the fact that each iteration should be better than the previous. Until further investigation I'd rather have them disabled so the CI is green.

@pramsey

This comment has been minimized.

Member

pramsey commented Feb 7, 2018

I'm not building the PR, this is just tests running in trunk.

@strk strk closed this in 5131a96 Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment