Skip to content
This repository has been archived by the owner on Dec 27, 2023. It is now read-only.

ISSUE-60: Fix for division by zero caused by float precision #61

Conversation

precariouspanther
Copy link
Contributor

Aims to resolve #60

Note: This fix is based on my assumption that for numbers where the $value_length < $in_scale + 1 the $value_log10_approx should be clamped to zero. This matches current test expectations (and log10 of the sample provided), but may be the incorrect place to address this (perhaps the natural scale for floats is the true source of this issue?)

Proof/Testing

New regression test covering example case and asserting that log10 precision for affected case returns expected value.

Ping @castarco

@castarco
Copy link
Contributor

castarco commented Aug 22, 2017

Have you checked what happens with numbers between 0.0 and 1.0 ? goes everything OK?

@precariouspanther precariouspanther force-pushed the ISSUE-60-float-innerlog10-div-zero branch from 5fe96b9 to 86cc13c Compare August 23, 2017 05:03
@precariouspanther
Copy link
Contributor Author

@castarco Yeah I did check, but I've updated the test to include a sample to verify that a number between 0.0 and 1.0 still behaves the same as prior to the change. The guard I've added only guards against negative numbers after the log approx. is calculated which would otherwise cause the division by zero (number pow negative = 0). This guard doesn't necessarily correct the behaviour (perhaps the approx log10 behaviour needs to take into account the natural precision vs just the values string length?) however does guard against the div by zero. I've manually verified several cases that we've encountered.

There is some inaccuracies with precision (the sample I've provided performs a 0.175 / 20.00 which should equal 0.00875 but instead rounds to 0.009, and the log10 operation has similar precision loss. However, this mirrors the current master behaviour as well so I set expectations based on this existing behaviour.

Is there some other specific sample cases you'd like my to verify?

@castarco castarco merged commit 9b90b32 into Coder-Spirit:master Aug 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Division by zero warning in innerLog10
2 participants