floating point FORMAT
Affects  Status  Importance  Assigned to  Milestone  

 SBCL 
Medium

Unassigned 
Bug Description
(format nil "~4,1F" 0.001) => "0.00" (should be " 0.0");
(format nil "~4,1@F" 0.001) => "+.00" (should be "+0.0").
(format nil "~E" 0.01) => "10.e3" (should be "1.e2");
(format nil "~G" 0.01) => "10.e3" (should be "1.e2");
Changed in sbcl:  
importance:  Undecided → Medium 
status:  New → Confirmed 
Changed in sbcl:  
assignee:  nobody → David Vázquez (davazp) 
David Vázquez (davazp) wrote :  #1 
Changed in sbcl:  
status:  Confirmed → Fix Committed 
status:  Fix Committed → Confirmed 
Roman Marynchak (romanmarynchak) wrote :  #2 
Hi David,
I briefly reviewed the patches, and they look good.
Two notes which I can give are based on my own SBCL patching experience (something like "newbie to newbie"):
1. It would be nice to add some simple regression tests in /tests/
2. You probably should change the bug status back to 'Confirmed'. 'Fix Committed' means that the patch is already in SBCL source tree.
Regards,
Roman
Nikodemus Siivola (nikodemus) wrote :  #3 
Thank you!
Test cases are sorely needed, yeah. Some random tests would be good too, and do much to increase confidence in the code. Apropos, did you check with ANSI test suite that this doesn't introduce new failures?
More detailed comments about why the fix is right would also be very welcome: reviewing the FP printing code is a pain, and likely to happen the faster the more detailed the explanation of changes is.
tags:  added: review 
David Vázquez (davazp) wrote :  #4 
All tests in tests/ directory are passed after of the patch. I include the following tests in tests/print.
;;; Tests for bug lp#308961
(assert (string= (format nil "~4,1F" 0.001) " 0.0"))
(assert (string= (format nil "~4,1@F" 0.001) "+0.0"))
(assert (string= (format nil "~E" 0.01) "1.e2"))
(assert (string= (format nil "~G" 0.01)) "1.00e2")
About a deeper explication. Well, as I said, the first bug is located in flonumtodigits function, which is a translation of a Scheme code. The algorithm implemented is supposed to work with positive numbers. However, the function was expanded in order to support rounding its input. So, since 0.001 is round to 0.0, the implemented algorithm fails. This require to understand the algorithm, but I test the lower limit of the interval (r  m) is positive simply, in order not to generate extra zeros.
The second bug is simpler to explain. formatexpaux scales a number such that it will have K digits before of the decimal point. This relies on scaleexponent. A example shows the trouble:
(format nil "~E" 0.1)
0: (FORMATEXPAUX #<SBIMPL:
1: (SBIMPL:
1: SBIMPL:
0: FORMATEXPAUX returned "1"
0.1 is 0.1 * 10^0 according to scaleexponent. Then, it is multiplied by 10^K before printing. This works well.
(format nil "~E" 0.01)
0: (FORMATEXPAUX #<SBIMPL:
1: (SBIMPL:
1: SBIMPL:
0: FORMATEXPAUX returned "2"
In this example, scaleexponent returns 1.0 and 2, meaning 0.01 = 1.0 * 10^2. Again, formatauxexp multiply it by 10^K. But as 1.0 has a digit before of the decimal point, the result will have K+1 digits.
This is due to formatexpaux assumed the returned value will be lesser to 1.0. In order to fix this, I decrement K when scaleexponent return 1.0 as primary value.
Greetings,
David.
Changed in sbcl:  
assignee:  David Vázquez (davazp) → Nikodemus Siivola (nikodemus) 
status:  Confirmed → In Progress 
tags:  added: floatingpoint 
Nikodemus Siivola (nikodemus) wrote :  #5 
Thank you! Sorry about taking so long to merge this.
Committed as:
commit 58187b3f2ab87bc
Author: David Vázquez <xxx>
Date: Mon Jun 28 17:28:08 2010 +0200
FLONUM
lp#308961, part 1.
SBCL has an extended version of the Burger & Dybwig fp printer, which
supports rounding.
It did not however support zero  but in eg.
(format nil "~,1F" 0.001)
0.001 is rounded to zero, which leads FLONUMTODIGITS giving us one zero too
many. Ie. it should be
=> "0.0"
but prior to this we got "0.00" instead.
This patch removes the special casing for 0, and instead tests that the lower
limit of the interval ( r m) is positive in order not to generate extra
zeros.
and
commit dafa18aa6bd65fe
Author: David Vázquez <xxx>
Date: Mon Jun 28 18:53:30 2010 +0200
FORMATAUXEXP: adjust scale if scaleexponent return 1.0
lp#308961, part 2.
FORMATEXPAUX scales a number such that it will have K digits before of the
decimal point. This relies on scaleexponent.
(format nil "~E" 0.1) ; => "1.e1"
0: (FORMATEXPAUX #<SBIMPL:
1: (SBIMPL:
1: SBIMPL:
0: FORMATEXPAUX returned "1"
0.1 is 0.1 * 10^0 according to scaleexponent. Then, it is multiplied by 10^K
before printing. Everything works out fine.
However!
(format nil "~E" 0.01) ; => "10.e3" ... oops
0: (FORMATEXPAUX #<SBIMPL:
1: (SBIMPL:
1: SBIMPL:
0: FORMATEXPAUX returned "2"
In this example, scaleexponent returns 1.0 and 2, meaning 0.01 = 1.0 *
10^2. Again, formatauxexp multiply it by 10^K. But as 1.0 has a digit
before of the decimal point, the result will have K+1 digits.
This is due to formatexpaux assumed the returned value will be lesser to
1.0. In order to fix this, we decrement K when scaleexponent return 1.0 as
primary value.
with tests in
commit bea52212cb68820
Author: Nikodemus Siivola <email address hidden>
Date: Tue Jun 7 12:29:21 2011 +0300
tests and NEWS for lp#308961
Also add a random FP read/print consistency tester.
For now it skips denormalized doublefloats due to bug 793774.
Changed in sbcl:  
assignee:  Nikodemus Siivola (nikodemus) → nobody 
status:  In Progress → Fix Committed 
tags:  removed: review 
Changed in sbcl:  
status:  Fix Committed → Fix Released 
I attach a patch for these bugs.
The first bug, corresponding to two first expressions, it is due to flonumtodigits tries to accept positive numbers,
however when it rounds, it could be zero. I fix flonumtodigits to accept nonnegative numbers.
The second bug, corresponding to two last expressions, it is due to formatexpaux assumes the value returned
from scaleexponent is in (0.1, 1.0), though 1.0 could be reached.