Replaced unsafe value < double.Epsilon expressions#1925
Replaced unsafe value < double.Epsilon expressions#1925pantosh wants to merge 1 commit intooxyplot:developfrom
Conversation
|
Azure failure can be ignored |
|
I'll leave this for the moment to let the other maintainers take a look, but if there are no objections I'll merged it after a few days. |
|
I tried to minimize the double.Epsilon changes to those that would surely fail on archs where 0 < double.Epsilon is false, however it should be considered to remove all usages of double.Epsilon. See the discussion in #1924. The largest breaking risk is in the HistogramSeries where it looks like double.Epsilon is used as some non-zero marker for min/max. That is risky on its own and should be changed, since it may evaluate to zero. |
|
For Historgram series, we probably need to detect the log(0) conditions explicitly and deal with it. Notes in #1886 |
|
Thanks for highlighting this issue! This is something I absolutely was not aware of. |
|
I'd agree with @Jonarw that (note: rebasing should stop azure whinging) |
|
The changes in this PR were the most conservative I could make, i.e. replacing < epsilon with <= epsilon. In conclusion, double.Epsilon is not a safe constant to use. For predicates that check if two calculations are mathematically the same "within floating point rounding error", some other epsilon must be used, i.e. 1e-50. Obviously this has its own problem, if the user happens to use data at that scale. Floats are not always easy to work with in a general purpose library. |
|
I think what @Jonarw and I are saying is that substituting I think the only truly problematic instance in the codebase is histogram (i.e. where just fixing it in this PR and forgetting about it won't cut it): in the other cases, there is no obvious epsilon to use, and picking one would just provide borderline-wrong behaviour for borderline data, so reducing them to |
|
(all that said, consistency probably isn't a big deal (not something we can solve), given the platforms will be inherently inconsistent anyway, and in the specific case of epsilon comparison, |
|
@pantosh sorry about my indecisiveness on this; if you can change the instances of As an aside, I had a bash at the histogram issue at some point but wasn't happy with it; will have another go once this and some of the modernisation PRs have gone through. |
Fixes #1924.
Changes proposed in this pull request:
All
Math.Abs(value) < double.Epsilonchecks replaced with<=since0 < double.Epsiloncannot be trusted to evaluate true on all architectures.