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

lwgeom_median: Avoid division by zero #198

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

Conversation

Projects
None yet
2 participants
@Algunenano
Member

Algunenano commented Jan 26, 2018

Algunenano added some commits Jan 26, 2018

lwgeom_median: Avoid division by zero
It could happen when an intermediate point is included in the set
and it's the geometry median. For example:
"MULTIPOINT ((0 -1), (0 0), (0 0), (0 1))"
@codecov

This comment has been minimized.

codecov bot commented Jan 26, 2018

Codecov Report

Merging #198 into svn-trunk will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##           svn-trunk     #198      +/-   ##
=============================================
- Coverage      79.32%   79.26%   -0.07%     
=============================================
  Files            201      201              
  Lines          62841    62956     +115     
=============================================
+ Hits           49846    49899      +53     
- Misses         12995    13057      +62
Impacted Files Coverage Δ
liblwgeom/lwgeom_median.c 97.22% <100%> (+0.05%) ⬆️
liblwgeom/cunit/cu_algorithm.c 98.99% <100%> (ø) ⬆️
liblwgeom/lwin_wkt_lex.c 43.06% <0%> (+0.52%) ⬆️

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 93decd1...73baf4a. Read the comment docs.

@strk strk closed this in 3bb8b93 Jan 26, 2018

"(0 -48000 -20000 1.3),"
"(0 -3000 -3472.22222222222262644208967685699462890625 1),"
"(0 3000 3472.22222222222262644208967685699462890625 1),"
"(0 0 -1644.736842105263121993630193173885345458984375 1),"

This comment has been minimized.

@Komzpa

Komzpa Jan 29, 2018

Member

I love this one. Really great, @Algunenano :)

This comment has been minimized.

@Algunenano

Algunenano Feb 1, 2018

Member

Sad news. For some reason (I'm guessing different float operation order) this hits in clang (5.0) but not in gcc (7.2.1, 6.4.1). I'm investigating the issue to try to find a common case

This comment has been minimized.

@Algunenano

Algunenano Feb 2, 2018

Member

Good news 😄 The issue was due to invalid rounding (-0.00000000000029917588874109483636135671651489707215 vs 0.0) in init_guess with the -ffloat-store flag that I guess clang just ignores.
I'll have a look to see if the flag is really useful.

@Komzpa

This comment has been minimized.

Member

Komzpa commented Feb 4, 2018

Hi @pramsey and @Algunenano,

since change 37bb96a the tests are red and the change was introduced in 3bb8b93#diff-245be84dc8debe637ec06a7838cccb96L1146

can something please be done about it?

@Algunenano

This comment has been minimized.

Member

Algunenano commented Feb 4, 2018

I'm investigating the issue. It appears that (0 * 0.5 + 0 * 0.5 + 48000 * 1.3 - 48000 * 1.3 - 3000 * 1 + 3000 * 1 + 0 * 1 + 0 * 1) doesn't result in 0 if the flag -ffloat-store is activated, but -0.0000000000002991758... instead so it takes a lot more iterations to end. Clang doesn't appear to support it so it was deactivated in all my local tests.

We can either remove the tests if the flag is active or deactivate the flag by default (and activate it only under some configure option) but I need to make some more tests before proposing a fix. I'll send a PR either today or tomorrow in any case.

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