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

switch BigFloat to MPFR & support MPC for complex bignums #2564

Closed
stevengj opened this issue Mar 14, 2013 · 14 comments
Closed

switch BigFloat to MPFR & support MPC for complex bignums #2564

stevengj opened this issue Mar 14, 2013 · 14 comments
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request

Comments

@stevengj
Copy link
Member

Not only would switching to MPFR improve the floating-point semantics, it would also add a whole raft of special functions etcetera. And the MPC library would allow corresponding support for Complex{BigFloat}.

@nolta
Copy link
Member

nolta commented Mar 14, 2013

I'm in favor of this. There was some discussion of switching to MPFR in #1221.

@StefanKarpinski
Copy link
Member

Yes, we should definitely do this.

@ViralBShah
Copy link
Member

Also, IIRC, bigfloat in GMP is no longer supported. MPFR is certainly the right way to go. Bot, MPFR and MPC are LGPL, which is nice too.

@andrioni
Copy link
Member

I've wrote a tentative wrapper of MPFR, and is is available here. The support for changing the precision is still a bit wonky, and I'm thinking about using a "context" system close to the one used by gmpy2.

Any comments?

@stevengj
Copy link
Member Author

Nice! Once this is working, I'd prefer to have this replace BigFloat. Some comments, in no particular order:

  • It's not completely clear to me what precision MPFRFloat(x::Real) should default to. In many cases, it seems like a better default would be min(DEFAULT_PRECISION, minimum precision to represent x exactly). In this case, a similar default would be used for binary operations '*' etcetera: the min(DEFAULT_PRECISION, min precision to represent result exactly). But maybe this wouldn't be easy to implement.
  • Would be nice to have a with_precision(p) do ... end construct that temporarily changes DEFAULT_PRECISION within its dynamic scope (similar to a fluid variable in Scheme).
  • Sometimes specifying the precision in bits and sometimes in digits is a tad confusing.
  • sqrt should probably throw a DomainError for negative inputs, for consistency with Julia. Similarly for ^.
  • Would be good to expose more of MPFR's special functions.
  • convert(::Type{Float32}, x::MPFRFloat) seems missing; not sure if there are other missing conversions.
  • isnan and isinf seem to be missing
  • sign (via mpfr_sgn?) seems missing. Also copysign (via mpfr_copysign) and exponent (via mpfr_get_exp) and modf (via mpfr_modf) and rem (via mpfr_remainder) and min/max (via mpfr_min/mpfr_max).
  • It is incorrect to implement '<=' etcetera in terms of cmp, because this mishandles NaN comparisons. Better to call mpfr_greater_p etcetera.
  • Might be worth calling mpfr_sum for + with more than 2 arguments (rather than calling the 2-argument mpfr_add repeatedly, as is the default from base/operators.jl).
  • Would be good to have nextfloat and prevfloat (from mpfr_nextabove and mpfr_nextbelow).
  • MPFR random-number functions would be good to have access to.
  • Julia's integer_valued could be implemented from mpfr_integer_p.

@andrioni
Copy link
Member

andrioni commented Apr 1, 2013

Thanks for the comments, @stevengj! Most of them have been implemented, and two issues were created to track some of them (special functions are on andrioni/MPFR.jl#1 and the rounding behavior is on andrioni/MPFR.jl#2).

Some comments:

  • I've decided to change all operations to always return a MPFRFloat of the current DEFAULT_PRECISION, for consistency, and the suggested with_precision function is now available.
  • The ^(x::MPFRFloat) seems to follow ^(x::Float64) in behavior right now, as both of them prefer to return a NaN instead of throwing a DomainError ((-1)^0.5 returns a NaN), but this is corrected in the case of sqrt.
  • Conversions to integers aren't implemented yet, as the default MPFR behavior is to round according to the given rounding mode, while Julia throws InexactError in the Float32/Float64 case and convert(::Type{Int64}, x::BigFloat) isn't defined.
  • I'm using the default sign defined at number.jl, as it conforms more to the rest of Julia (returns the same type as the argument, etc).
  • Comparisons taking NaN in account are now implemented too, as are copysign, exponent, nextfloat, prevfloat and integer_valued.

@stevengj
Copy link
Member Author

stevengj commented Apr 1, 2013

Great!

Julia only throws an InexactError for conversion from float to integers if the floating-point value is not an integer in the representable range of the target type. convert(Int, 2.0) works just fine. The same thing should be true for MPFR.jl: it should throw an InexactError if the MPFR value is not an integer in the representable range of the target type, but it should work fine if the conversion can be performed exactly.

@andrioni
Copy link
Member

I think MPFR.jl is good enough to be packaged. It has more functions and utilities than BigFloat (including basic ones like exp, log and the trigonometric functions), it has a somewhat simple, yet useful system to control the precision, and I had no big issues working with it.
I'll write some documentation and then send a pull request to METADATA, probably tomorrow, and then return to work more actively on MPC.jl, which depends on MPFR.jl being available.

@JeffBezanson
Copy link
Member

You should make a pull request to replace BigFloat, instead of making a new package.

@ViralBShah
Copy link
Member

Yes, this should go into Base. We also need to add MPFR and MPC to the build process in deps.

@andrioni
Copy link
Member

I added MPFR to the build process in andrioni/julia@146901a and andrioni/julia@f9423dd and it built fine here yesterday.

@andrioni
Copy link
Member

andrioni/julia@786749c adds MPC as a dep. If you want, I may send a pull request just for this or as a RFC for the MPFR/MPC integration.

@ViralBShah
Copy link
Member

Yes, please make it a pull request to Base, and put RFC in the title of the request, so that people will review it.

@JeffBezanson
Copy link
Member

MPFR wrapper merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

6 participants