-
Notifications
You must be signed in to change notification settings - Fork 105
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 floating point precision failures caused by FMA on ppc64le #213
Conversation
Thank you very much for this :) The CI failure is not related to changes in this PR. I'll fix those shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. Please can you:
- Add a test that demonstrates the fix (the test should fail on
ppc64le
for the code before the change and pass onppc64le
for the fixed code). - Remove any unnecessary casts to
float64
.
If this a bug in FMA, is this reported upstream?
xy/area_centroid.go
Outdated
calc.cg3[0] += sign * area2 * calc.triangleCent3[0] | ||
calc.cg3[1] += sign * area2 * calc.triangleCent3[1] | ||
calc.areasum2 += sign * area2 | ||
calc.cg3[0] += float64(sign * area2 * calc.triangleCent3[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the variables here (sign
, area2
, and calc.triangleCent3[0]
) are already float64
s, so what effect does the cast to float64
have?
Same question for the rest of the changes in this function.
xy/internal/cga.go
Outdated
@@ -60,5 +60,5 @@ func Distance2D(c1, c2 geom.Coord) float64 { | |||
dx := c1[0] - c2[0] | |||
dy := c1[1] - c2[1] | |||
|
|||
return math.Sqrt(dx*dx + dy*dy) | |||
return math.Sqrt(float64(dx*dx) + float64(dy*dy)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the casts to float64
needed here?
Same question for the other casts in this PR.
The listed tests fail on ppc64le.
With explicit casts in this patch, these failures are resolved.
Please also see: |
7766786
to
6b44180
Compare
6b44180
to
0f92480
Compare
Following tests fail on ppc64le due to FMA: xy - TestAreaGetCentroid - TestCentroid - TestSignedArea - TestLineGetCentroidLines - TestLineGetCentroidPolygons - ExampleLinearRingsCentroid xy/lineintersector - TestRobustLineIntersectionLines xyz - TestVectorDot - TestVectorLength - TestDistanceLineToLine - TestDistancePointToLine See https://go.dev/ref/spec#Floating_point_operators for more details.
0f92480
to
347fcad
Compare
Thank you for the follow-up and for the pointers to Go's spec around floating point. Today I learned :) |
Thanks very much for this fixes. I've tagged |
@twpayne, Thank you for fixing the formatting. My local run didn't complain about it. |
Following tests fail on ppc64le due to FMA:
xy
- TestAreaGetCentroid
- TestCentroid
- TestSignedArea
- TestLineGetCentroidLines
- TestLineGetCentroidPolygons
- ExampleLinearRingsCentroid
xy/lineintersector
- TestRobustLineIntersectionLines
xyz
- TestVectorDot
- TestVectorLength
- TestDistanceLineToLine
- TestDistancePointToLine
See https://go.dev/ref/spec#Floating_point_operators for more details.