Synethetic Source: Fix scaled float#86760
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| } | ||
| return min; | ||
| } | ||
| return scaledValue / scalingFactor; |
There was a problem hiding this comment.
Given this logic is fairly gnarly, is it worth extracting it into a helper and then throwing some heavy-duty testing on it? Even something quick-check like, to test boundary conditions and then a solid random selection of points.
There was a problem hiding this comment.
We get that from the round trip tests but I could certainly see wanting it to be tested more directly. I'll yank it around a bit.
|
@romseygeek I've pushed some more tests. Let me know if you think I should add even more. This is a tricky thing. |
romseygeek
left a comment
There was a problem hiding this comment.
LGTM. Let's get it in and see what CI makes of it.
not-napoleon
left a comment
There was a problem hiding this comment.
LGTM. Or, well, looks as good as this much floating point jank can look ;) Nice work.
| * double decoded = scaled1 / scalingFactor; | ||
| * long scaled2 = Math.round(decoded * scalingFactor); | ||
| * assert scaled2 != Long.MAX_VALUE; | ||
| * }</pre> |
There was a problem hiding this comment.
Good explanation, thanks for including it.
|
Be free PR! Go and mingle with your brothers and sisters in CI and find bugs. |
This causes scaled float values that entirely saturate their numeric
range to continue saturating their range on round trip.