Skip to content

Add the constant pi and related values#6

Closed
Chris00 wants to merge 4 commits intojanestreet:masterfrom
Chris00:master
Closed

Add the constant pi and related values#6
Chris00 wants to merge 4 commits intojanestreet:masterfrom
Chris00:master

Conversation

@Chris00
Copy link
Copy Markdown

@Chris00 Chris00 commented Dec 8, 2016

The addition of the constant to Pervasives has been requested several
times but it is better that these constants do not clutter the default
namespace by being in the Float module.

https://caml.inria.fr/mantis/view.php?id=4170
https://caml.inria.fr/mantis/view.php?id=5173

The addition of the constant to Pervasives has been requested several
times but it is better that these constants do not clutter the default
namespace by being in the Float module.

https://caml.inria.fr/mantis/view.php?id=4170
https://caml.inria.fr/mantis/view.php?id=5173
@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Dec 8, 2016

This seems great. Can you explain a bit the justification of the specific set of values proposed? e.g., why sqrt(pi)?

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 8, 2016

1/π and √π are needed to, say, normalize the Fourier coefficients and the Fourier Transform — for the latter, actually 1/√(2π) but I did not want to add to many constants (maybe a separate √2 is useful?).

Another thing to possibly discuss: I used names with underscores — as I find them clearer — but √π is sometimes written sqrtpi.

@ghost
Copy link
Copy Markdown

ghost commented Dec 8, 2016

BTW, shouldn't these use the hexadecimal float notation that was added recently? I think it's supposed to be more reliable

@agarwal
Copy link
Copy Markdown

agarwal commented Dec 8, 2016

1/π and √π are needed to, say, normalize the Fourier coefficients and the Fourier Transform

Okay, but why can't they be computed when needed? Are they needed so often that avoiding the computation is worthwhile from a performance perspective? Or is there some numerical sensitivity issue you're accounting for that makes it better to give the exact values you provide?

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 8, 2016 via email

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 8, 2016

@diml One can hope that the compiler does a correct rounding when parsing numbers but maybe you would like to add some tests? Using the hexadecimal notation in the library itself will make it work only for recent versions of OCaml or require conditional code.

@ghost
Copy link
Copy Markdown

ghost commented Dec 9, 2016

Base already requires OCaml >= 4.03. I didn't check but I'm pretty sure we are already using 4.03 specific features in Base. So it's not really a problem to require support for the hexadecimal notation.

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Dec 10, 2016

@Chris00 Is pi_2 really better than pi /. 2.? The meaning of the former is unclear from the name (pi_over_2 would be clear enough, but now clearly less concise).

sqrt_pi is clear, but no clearer than sqrt pi. Similarly, inv_pi is clear, but not obviously better than 1. /. pi. Is the motivation for these more about precision or performance? Certainly division and sqrt are expensive operations.

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 10, 2016

sqrt_pi in indeed slightly more precise (sqrt_pi -. sqrt pi ≈ 2.22e-16) and less costly (but of course, one can always declare it in a given module). So the reason for √π is mostly precision [with it, sqrt 2. *. sqrt_pi gives the best approximation of √(2π) — I just checked and sqrt 2. *. sqrt pi works as well, but not sqrt(2. *. pi)].

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 10, 2016

So maybe it is better to add sqrt_2pi instead of sqrt_pi.

src/float.mli Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like the mli doesn't current match the ml.

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Dec 10, 2016

For example, 1. /. pi = 0x0.517cc1b727220a94fe13abe8fa9a6e
@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 10, 2016

See also the GSL.

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Dec 22, 2016

See also Gg.

@mshinwell
Copy link
Copy Markdown

Please also see Richard Jones's comment in https://caml.inria.fr/mantis/view.php?id=4170

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Feb 9, 2017

Sorry for letting this hang out for so long. I'm pushing this through our internal process.

@cshardin
Copy link
Copy Markdown

cshardin commented Feb 14, 2017

(Edited in response to question below.)

One can hope that the compiler does a correct rounding when parsing numbers but maybe you would like to add some tests?

This was my thought as well, but it appears that decimal numbers are not reliably rounded correctly. The problem is that caml_float_of_string uses C's strtod, whose correctness is platform-dependent. Googling strtod implementations, the first one I found had this comment: "If the mantissa has more than 18 digits, ignore the extras, since they can't affect the value anyway." (That's untrue for numbers that are close to halfway between representable values.) On my own machine, about 1 in 50,000 values gets rounded the wrong way. The concern for me is the not the negligible loss of precision but the nondeterminism that comes from different machines having versions of strtod that disagree about the mapping of strings to doubles. (Thankfully the scope of this problem is pretty narrow, because presumably they do agree about how to round strings that result from converting a double to a string.)

@Chris00
Copy link
Copy Markdown
Author

Chris00 commented Feb 14, 2017

@cshardin You are speaking of decimal representations, right?

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Feb 14, 2017 via email

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Feb 16, 2017

This has been released internally, and will land in the next bleeding-edge release.

@yminsky yminsky closed this Feb 16, 2017
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.

5 participants