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

Implement erfinv for Float32 and Float64 #1344

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

Conversation

ararslan
Copy link

The implementations are based on the Julia package SpecialFunctions.jl, which uses a paper from Blair et al. (1976). This is the first part of the first item in #1063.

This is my first time using Dex so apologies if the code looks strange. 😅 I'm happy to make any changes.

The implementations are based on the Julia package SpecialFunctions.jl.
Copy link

google-cla bot commented Dec 31, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@@ -1066,6 +1067,127 @@ def float64_cosh(x:Float64) -> Float64 = %fdiv(%fadd(%exp(x), %exp(%fsub(f_to_f6
def float64_tanh(x:Float64) -> Float64 = %fdiv(%fsub(%exp(x), %exp(%fsub(f_to_f64 0.0, x)))
,%fadd(%exp(x), %exp(%fsub(f_to_f64 0.0, x))))

# Polynomial evaluation by Horner's method
def unsafe_horner(x:a, ys:n=>a) -> a given (a|Add|Mul, n|Ix) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This duplicates the evalpoly function in the same file, though yours is maybe a little more general since it doesn't require a zero.

https://github.com/google-research/dex-lang/blob/main/lib/prelude.dx#L2225

Copy link
Author

Choose a reason for hiding this comment

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

Ah right, thanks for pointing this out, I should have left a comment about it somewhere. The duplication is unfortunate but was intentional, because:

  • evalpoly, nor any of the reduction machinery, isn't yet defined at this point in the prelude, and writing out a polynomial evaluation by Horner's method by hand with a lot of coefficients in multiple branches is quite tedious. (I believe one of the other special functions does this.) I was unsure about the ramifications of shuffling around the contents of the prelude to facilitate, so I left things in place.
  • evalpoly requires a VSpace and Float but we want to be able to support both Float (AKA Float32) and Float64, but the latter doesn't implement VSpace (see also Can't negate a Float64 #1345).

I tried to differentiate it from evalpoly as more of an internal helper than an intended user-facing function in part by prefixing it with unsafe_, which seemed appropriate since it directly uses two functions similarly marked as unsafe.

though yours is maybe a little more general since it doesn't require a zero.

As an aside, IIUC, I think mine does still require a zero since that's part of the Add interface and I have a|Add.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, thanks for explaining. Yes, moving things within the prelude is a punishing game of dependency tetris. @dougalm has usually been fine with me moving things around as long as the tests still pass, but I would be surprised if you could move evalpoly up 1000 lines and have it still work.

Good point about the zero, I got mixed up about which method is in which class.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it might take a bit of additional shuffling and/or finagling, but I think the Floating interface and its instances could be moved down below evalpoly. That way they'd also be able to use copysign, inf, and nan (at least for the Float32 implementation, anyway). I believe the only use of Floating prior to the definition of evalpoly is for std, which would need to be moved as well. Though even if this shuffling is done, evalpoly itself would need to be amended in order to be usable for the Float64 definition.

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.

2 participants