Skip to content
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

Protect agains infinite loop when plotting tiny ranges #130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghorn
Copy link
Contributor

@ghorn ghorn commented Jun 19, 2016

simple fix for #128

@@ -108,7 +108,7 @@ scaledAxis lap rs@(minV,maxV) ps0 = makeAxis' realToFrac realToFrac
ps = filter isValidNumber ps0
range [] = (0,1)
range _ | minV == maxV = if minV==0 then (-1,1) else
let d = abs (minV * 0.01) in (minV-d,maxV+d)
let d = max 1e-300 (abs (minV * 0.01)) in (minV-d,maxV+d)
| otherwise = rs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure if this is the correct behavior. It seems to give me a range of (-1e-300, 1e-300) when rs = (0, 0) and the freeze came back. Could you check it for me?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 1e-300 is bad choice. Smallest normalized number 2.2250738585072014e-308 is 8 orders of magnitude away and underflow doesn't happen even for 1e-321. It's probably better to check whether maxV+d and minV-d are equal to maxV/minV.

range _   | minV == maxV = case minV of
             0                            -> (-1,1)
             _ | lo /= minV && hi /= maxV -> (lo,hi)
               | otherwise                -> (minV/2,maxV*2)
          | otherwise    = rs
d  = abs (minV * 0.01)
lo = minV - d
hi = maxV + d

@timbod7
Copy link
Owner

timbod7 commented Jun 30, 2016

I've pushed a fix to avoid the freeze in this commit: f27955b

The issue is that this expression:

10 ^^ ((floor $ log10 $ delta / nsteps)::Integer

will eat all RAM in bigint calculations when delta = 0

@timbod7
Copy link
Owner

timbod7 commented Jun 30, 2016

This doesn't guarantee you'll get sensible charts with tiny values - your patch may still be necessary for this. Test it and if it is necessary, I will merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants