jk-jeon / dragonbox

Reference implementation of Dragonbox in C++
Apache License 2.0
607 stars 39 forks source link

Improve paper #56

Closed gifti258 closed 8 months ago

gifti258 commented 8 months ago

I recently implemented the main algorithm in Factor with the paper as my main source.

My two main hurdles were these:

Another, smaller grievance is in section 5.2.3:

P.S.: In section 5.1.7 for completeness, you could mention that you compute r as r = z^(i) - 10^(κ+1)*s. This might seem like baby steps, but I had to extract the obvious from another implementation.

jk-jeon commented 8 months ago

Wonderful, many thanks!

There is a typo Algorithm 5.6, steps 6–8: where there is 10^(k_0), it should be 10^(-k_0). I could only find this by looking at the reference implementation.

Thanks, fixed.

In Algorithm 5.4, step 5, or in the text, very little is said about how to break the tie. The code is a little obfuscated, the significand is 10s̃ + t, not f_c, as I first thought. Also helpful would be the information that you choose y^(rd) if the significand is odd, for the nearest-to-even case.

I added a footnote about this in page 17 and made some comments on the reference implementation.

  • The information that d_1, d_2 are always zero is hidden away very well.
  • The upper bound for e in the integer check of x̃^(i) and z̃^(i) is a constant. It evaluates to 3 in the case of binary32 and binary64.

Maybe it's just me, but I tend to read all attached footnotes immediately after I complete reading a sentence, so I don't find it very much hidden. Also I think it's quite clear that $2^{p+2}-1$ and $2^{p+1}+1$ are constants, so it's also obvious that $d{1},d{2}$ are constants once the reader finds out their definition, so I think even if the reader missed the footnotes they could find out $d{1},d{2}$ easily by themselves. So I decided to not touch this part.

P.S.: In section 5.1.7 for completeness, you could mention that you compute r as r = z^(i) - 10*s. This might seem like baby steps, but I had to extract the obvious from another implementation.

Fair enough, edited accordingly. (I assumed you meant $r = z^{(i)} - 10^{\kappa+1}s$.)

Again, I appreciate it very much that you reviewed the paper. Few people did so far. I'm also quite glad that someone implemented this by actually reading the paper rather than copy-paste-modifying the reference implementation. Having multiple different implementations is a great way to cross-check both the paper and the implementations together.

If you have no further comments right now, please close the issue. And also please let me know if you find anything unclear or suspicious or erroneous afterwards, thanks!

gifti258 commented 8 months ago

In Algorithm 5.4, step 5, or in the text, very little is said about how to break the tie. The code is a little obfuscated, the significand is 10s̃ + t, not f_c, as I first thought. Also helpful would be the information that you choose y^(rd) if the significand is odd, for the nearest-to-even case.

I added a footnote about this in page 17 and made some comments on the reference implementation.

I think that helps a lot.

  • The information that d_1, d_2 are always zero is hidden away very well.
  • The upper bound for e in the integer check of x̃^(i) and z̃^(i) is a constant. It evaluates to 3 in the case of binary32 and binary64.

Maybe it's just me, but I tend to read all attached footnotes immediately after I complete reading a sentence, so I don't find it very much hidden. Also I think it's quite clear that 2p+2−1 and 2p+1+1 are constants, so it's also obvious that d1,d2 are constants once the reader finds out their definition, so I think even if the reader missed the footnotes they could find out d1,d2 easily by themselves. So I decided to not touch this part.

It's not directly hidden, but I think the reader could benefit if the footnotes were included directly in the text, at the end of the section, with the addition that for binary32 and binary64, the upper bound evaluates to 3 or that x is an integer iff 2 ≤ e ≤ 3 and z is an integer iff 0 ≤ e ≤ 3.

P.S.: In section 5.1.7 for completeness, you could mention that you compute r as r = z^(i) - 10*s. This might seem like baby steps, but I had to extract the obvious from another implementation.

Fair enough, edited accordingly. (I assumed you meant r=z(i)−10κ+1s.)

You are of course right.

jk-jeon commented 8 months ago

It's not directly hidden, but I think the reader could benefit if the footnotes were included directly in the text, at the end of the section, with the addition that for binary32 and binary64, the upper bound evaluates to 3 or that x is an integer iff 2 ≤ e ≤ 3 and z is an integer iff 0 ≤ e ≤ 3.

Actually, that sounds very reasonable. I edited it accordingly in 0cba14ca5aea98c0c6f525561a30676fd28159a6. Thanks for the input!