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

fix bugs in compute_right_closed_directed #22

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Conversation

yotann
Copy link
Contributor

@yotann yotann commented Sep 30, 2021

These bugs only affect the rounding modes toward_plus_infinity, toward_minus_infinity, and away_from_zero.

In the paper, Algorithm A.1 step 7 currently says:

x = (fc - 1/2) * 2^e * 10^k = (2*fc - 1) * 2^{e-1} * 10^k
for the normal interval case

x = (fc - 1/4) * 2^e * 10^k = (4*fc - 1) * 2^{e-2} * 10^k
for the shorter interval case

These are incorrect for right-closed directed rounding, which uses w-; see the definition of w- in section 2.2. The correct values are:

x = (fc - 1) * 2^e * 10^k = (2*fc - 2) * 2^{e-1} * 10^k
for the normal interval case

x = (fc - 1/2) * 2^e * 10^k = (2*fc - 1) * 2^{e-1} * 10^k
for the shorter interval case

Test program

#include <iomanip>
#include <iostream>
#include <limits>

#include "dragonbox/dragonbox.h"

template<typename Float>
static void away_from_zero(Float f) {
    auto v = jkj::dragonbox::to_decimal(f, jkj::dragonbox::policy::decimal_to_binary_rounding::away_from_zero);
    std::cout << std::scientific;
    std::cout << std::setprecision(std::numeric_limits<Float>::digits10 + 5);
    std::cout << f << ": " << v.significand << "E" << v.exponent << "\n";
}

int main(int argc, char **argv) {
    away_from_zero(5e-324);
    away_from_zero(3.33e-321);
    away_from_zero(3.335e-321);
    away_from_zero(1.547425049106725e+26);
    away_from_zero(1.5474250491067252e+26);
    away_from_zero(35184369991680.0f);
    away_from_zero(35184372088832.0f);
    return 0;
}

Incorrect output

4.94065645841246544177e-324: 0E-307
3.33000245297000170775e-321: 333E-323
3.33494310942841417319e-321: 333E-323
1.54742504910672500003e+26: 1547425049106725E11
1.54742504910672517183e+26: 1547425049106725E11
3.51843699917e+13: 35184369E6
3.51843720888e+13: 35184372E6

Fixed output

4.94065645841246544177e-324: 4E-324
3.33000245297000170775e-321: 333E-323
3.33494310942841417319e-321: 3334E-324
1.54742504910672500003e+26: 1547425049106725E11
1.54742504910672517183e+26: 15474250491067251E10
3.51843699917e+13: 35184369E6
3.51843720888e+13: 3518437E7

In the paper, Algorithm A.1 step 7 currently says:

`x = (fc - 1/2) * 2^e * 10^k = (2*fc - 1) * 2^{e-1} * 10^k`
for the normal interval case

`x = (fc - 1/4) * 2^e * 10^k = (4*fc - 1) * 2^{e-2} * 10^k`
for the shorter interval case

These are incorrect for right-closed directed rounding, which uses w-;
see the definition of w- in section 2.2. The correct values are:

`x = (fc - 1) * 2^e * 10^k = (2*fc - 2) * 2^{e-1} * 10^k`
for the normal interval case

`x = (fc - 1/2) * 2^e * 10^k = (2*fc - 1) * 2^{e-1} * 10^k`
for the shorter interval case
@jk-jeon jk-jeon merged commit a54232b into jk-jeon:master Sep 30, 2021
@jk-jeon
Copy link
Owner

jk-jeon commented Sep 30, 2021

Thanks a lot for catching this!

jk-jeon added a commit that referenced this pull request Oct 1, 2021
yotann added a commit to yotann/bcdb-private that referenced this pull request Mar 9, 2022
9c26179a Fix jk-jeon/dragonbox#27
02059bde Fix a typo
57d9b60c Fix a bug
ba65f0ef Add missing copyright notice
fcc6cc0e Improve comment
29aa8a23 Improve comment
fa139ad9 Help MSVC to not generate horrible assembly
478edcc4 Fix ABI overhead issue
d4f164bd Improve comment
a84c11e1 A small optimization turning mul into lea
6bf01402 Add the old paper along with the new one
955d7387 Rerun Milo's benchmark
77aeee25 Refactoring + some improvement on 9-digits case
b75d296c Optimize digit printing algorithm
8864215b Merge branch 'master' of https://github.com/jk-jeon/dragonbox
dd52aa72 Workaround clang-format & correctly attribute jeaiii
71684f08 Update README.md
21400aab Rewrite paper
3da7d9d8 Change <= epsilon to < epsilon, as that's what we need
4b1f309d Update README.md
fefd03b4 Rerun Milo's benchmark
5ca7e3d2 Change the order of condition evaluations
5ddb8bcf Add more const; reformat
b4d5f705 Add test for compressed cache
fb970736 Fix incorrect analysis; now it is correct.
fe2e5c1e Rename beta_minus_1 to beta to reflect the  new paper
2b673c0c Rerun benchmark
3f33d4dc Run MATLAB even when no actual benchmark has been done
4d82dfe4 Fix a bug that shaded regions are not correctly shown in the exported pdf
e52fe78c Use better exportgraphics rather than print
f1e59661 Merge branch 'master' of https://github.com/jk-jeon/dragonbox
2e6eefa5 Update README.md
0377186d Update README.md
38e11bc4 Rerun verify_cache_precision
07a450ed Ignore pdf files generated from plot_required_bits.m
ef4ac121 Required bits plot MATLAB script
d4f0f783 Reflect the new paper & add file output
eff5914f Reflect the new paper
bba1a8b7 Merge branch 'master' of https://github.com/jk-jeon/dragonbox
a37c3c28 Add missing test cases. No problem found.
96dc15c8 Update README.md
d41e062b Fix a bug
83d5df3e Reflect the new paper, simplify things
692f67f4 Implement jeaiii-style printing algorithm
cd533cde Both binary32 and binary64 remove trailing zeros
af08b920 Fix an error (has no effect in fact)
133f0aaf Match new paper
f5ff2c4e Shall not feed 0 to dragonbox
2537b740 Further simplify check_divisibility_and_divide_by_pow10
4cece761 Even more simplified version of check_divisibility_and_divide_by_pow10
274d4634 Use newly implemented find_all_good_rational
990b2a4f Add file
028083c8 Rename
92001c41 Implement find_all_good_rational_approx_from_below_denoms
2ce3f894 Add some utilities to unsigned_rational
15e6a56d Add a little more utility functions
997a4fda Fix typos and improve some sentences
deabce23 Fix typos, improve some sentences, correct some small errors
2ee87104 Improve comments
812646d3 Improve comments
8c484269 Include missed cases
72e22a33 Include possibily missed cases
66de8584 Add template keyword here and there
1c1ca394 More precise test
dc41a327 Refine false positive condition
3e1083a2 Remove generate_compressed_cache_error_table
938a3ed6 Implement compressed cache verification
259787da Simplify compressed cache policy
569980d5 Justify why we don't need to add the error back.
d7e8e307 Fix errors
23c2c20b Carefully re-done
f75e50af Reformat
1f452fa8 Add is_even()
4a3402db Fix error
369cd0bc Improve comments and other conventions
30d9e4eb Simplify check_divisibility_and_divide_by_pow10. Apply the integer check idea from Schubfach.
4150375b Right-shift for signed integers is implementation-defined See fmtlib/fmt#2709
1c7596b1 Addressing jk-jeon/dragonbox#25
2e3f58c2 Adapt the change in dragonbox.h
8a1c13d7 Change header guards convention
ec688b1a Remove now-unused stuffs
b9f01c18 Reformat
1bfe1176 #if block for ease of testing
3c0f1c6f Eliminate integer checks
dc4557df Reformat
ac5788da Adapt new table generation scheme
6251ad9a Reformatting
346f7538 Use new bignum
aa5c699a Regenerate cache
c0776eec Add dragonbox::common as a dependency to sandbox
bb47e99e Rewrite to match the new method
daa0c81e Fix some trivial errors
a29a8efb Modify copyright
9b11d788 Rename a file
a5316aa0 Rename
bc3d0eaf Modify according to the new scheme
74a58cb8 Remove old files
b4a66924 Big int & continued fractions implementation
6281dc3e Add dragonbox::common to dependency
9868af72 Fix typo in comment
0cdb1548 Add dragonbox_to_chars as a dependency to sandbox
9b460165 Merge pull request #24 from jwakely/patch-1
4f3715d6 Fix typos and grammar in README
cf8b12cc Rerun milo's benchmark
1a964604 Rename main.cpp into benchmark.cpp
ab23f7aa Simplify remove_trailing_zeros further
bf8442be Rename main.cpp into benchmark.cpp
ba521e30 It seems the new version of readtable now reads hex integers directly
5ecd1534 Fix copyright notice
106f7b94 Replace stof/d into strtof/d
12310e74 Remove unused param name
11aae947 Improve comments on ROR
99fd14ba Fix the bug introduced by the commit e26c8d7363eb0ad9753b56a9699dea194784eb5e
648591a4 Fix a bug introduced by commit 2a33d79e1ce6f2045edacd79c8f087ffd10e1323
aad99d52 Silence MSVC warning by defining JKJ_DRAGONBOX_HAS_BUILTIN
39d32f1d Fix typo
2db8656d Fix typo
e26c8d73 Optimize/simplify remove_trailing_zeros
2a33d79e Optimize break_rounding_tie Now the policy classes do not actually round, rather, it returns a boolean flag for if they may want to round
d9b2664d Optimize check_divisibility_and_divide_by_pow10
5a699783 Reduced usages of __int128 __int128 seems to generate worse code
2ac7fa6a Add minmax Euclid to the paper
db39c05c Fix typo
f3764d5a Change divtable from SOA-style into AOS-style Because it makes more sense
63d9dc1b Update paper according to jk-jeon/dragonbox#22
8323f379 Make README.md clearer
a54232b9 Merge pull request #22 from yotann/master
fc909b8d fix bugs in compute_right_closed_directed
16473a7d Merge branch 'master' of https://github.com/jk-jeon/dragonbox
538d7d32 Revised some writings
90face27 Merge pull request #21 from Alexhuszagh/master
e9509370 Add CI support for OSes and other architectures.

git-subtree-dir: third_party/dragonbox
git-subtree-split: 9c26179a9c4368db9f7703879e3b2e34aa0e29c2
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.

None yet

2 participants