[RFC] MPFR and MPC on Base #2814

Merged
merged 31 commits into from May 3, 2013

Conversation

Projects
None yet
7 participants
@andrioni
Member

andrioni commented Apr 10, 2013

Hello,

as it was discussed in #2564, I am doing this pull request with a tentative integration of both MPFR and MPC on Base. It is currently working and passes all tests (I used the same tests I wrote when they were in packages), so it's easy for anyone to try it.

Known issues:

  • make check for MPFR and MPC runs their test suites, lengthening considerably the build process, as make check is always called, and I have yet to discover how to bypass this.
  • There is no written documentation currently, nor examples other than the tests.
  • Support for different rounding modes.
  • Renaming MPFRFloat to BigFloat.
  • Renaming precision-related functions
  • convert methods for BigComplex
  • Migration to a direct representation of the structs involved, instead of a bunch of memory allocated as an array
    • BigInt
    • BigFloat
  • mpfr_hypot
  • Better basic operations rules between BigTypes and primitives to avoid using promotion to deal with it
  • Set 256 as the default precision.
  • Add the needed -dev libraries to .travis.yml
  • Reimplement sum for MPFRFloat in a way that works, as it needs an array of pointers.
  • MPFR has no make distclean option, it seems.
@ViralBShah

This comment has been minimized.

Show comment Hide comment
@ViralBShah

ViralBShah Apr 11, 2013

Member

@staticfloat Do you think we can support these in travis before we merge this?

Member

ViralBShah commented Apr 11, 2013

@staticfloat Do you think we can support these in travis before we merge this?

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

The types should probably be called BigFloat (replacing the current type of the same name) and BigComplex, rather than MPFRFloat and MPCComplex. (Which libraries are used to implement a given functionality should be mostly invisible to the user.)

Correspondingly, I would use get_bigfloat_precision etcetera to get/set the precision. The short names prec and prec2 should probably be ``get_bigcomplex_precision(x::BigComplex)(and just dompc_get_prec2`), or have a generic `get_precision` that works for all floating-point types.

You don't need iscomplex(::MPCComplex) = true since true is the default for any subtype of Number.

Why isn't there a convert(::Type{Complex{Float64}}, x::MPCComplex) function etcetera?

Member

stevengj commented Apr 11, 2013

The types should probably be called BigFloat (replacing the current type of the same name) and BigComplex, rather than MPFRFloat and MPCComplex. (Which libraries are used to implement a given functionality should be mostly invisible to the user.)

Correspondingly, I would use get_bigfloat_precision etcetera to get/set the precision. The short names prec and prec2 should probably be ``get_bigcomplex_precision(x::BigComplex)(and just dompc_get_prec2`), or have a generic `get_precision` that works for all floating-point types.

You don't need iscomplex(::MPCComplex) = true since true is the default for any subtype of Number.

Why isn't there a convert(::Type{Complex{Float64}}, x::MPCComplex) function etcetera?

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 11, 2013

Member

I will do the replacements and renamings right now.

The convert functions are lacking mainly because I was finishing them in MPFR (as MPC is just a library around a pair of MPFR floats). Implementing them will be quite direct now.

I'll add these issues to the checklist on the top, and update it as I do them, thanks!

Member

andrioni commented Apr 11, 2013

I will do the replacements and renamings right now.

The convert functions are lacking mainly because I was finishing them in MPFR (as MPC is just a library around a pair of MPFR floats). Implementing them will be quite direct now.

I'll add these issues to the checklist on the top, and update it as I do them, thanks!

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 11, 2013

Member

Also, one question: I'm currently using 53 bits as the default precision (i.e. a Float64), but BigFloat currently uses the GMP default, 64 bits. Should I up the default to match it (for compatibility), or should I use a higher number (maybe 256), to give a nice precision by default for those who don't want to mess with it?

Member

andrioni commented Apr 11, 2013

Also, one question: I'm currently using 53 bits as the default precision (i.e. a Float64), but BigFloat currently uses the GMP default, 64 bits. Should I up the default to match it (for compatibility), or should I use a higher number (maybe 256), to give a nice precision by default for those who don't want to mess with it?

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

I would default to 256-ish (~octuple) precision.

It would be much nicer to implement BigComplex as Complex{BigFloat}, so that you share all of the Complex{T} methods. You can still call MPC for the operations as needed. In fact, if you do it right (make BigFloat an immutable wrapper around a single mpfr_t pointer), Complex{BigFloat} should even be bit-compatible with mpc_t, so you won't even need to do a conversion (although making an mpc_t on the fly as needed should be cheap).

I'm confused: why is MPFRFloat.mpfr a Vector{Int32}, instead of being an opaque mpfr_t pointer?

Member

stevengj commented Apr 11, 2013

I would default to 256-ish (~octuple) precision.

It would be much nicer to implement BigComplex as Complex{BigFloat}, so that you share all of the Complex{T} methods. You can still call MPC for the operations as needed. In fact, if you do it right (make BigFloat an immutable wrapper around a single mpfr_t pointer), Complex{BigFloat} should even be bit-compatible with mpc_t, so you won't even need to do a conversion (although making an mpc_t on the fly as needed should be cheap).

I'm confused: why is MPFRFloat.mpfr a Vector{Int32}, instead of being an opaque mpfr_t pointer?

@StefanKarpinski

This comment has been minimized.

Show comment Hide comment
@StefanKarpinski

StefanKarpinski Apr 11, 2013

Member

I'm confused: why is MPFRFloat.mpfr a Vector{Int32}, instead of being an opaque mpfr_t pointer?

I don't know if it's the case here, but in cases where a fixed amount of memory is used to store some C blob, you can make the thing serializable by using a Julia array for the storage. E.g. that's how compiled PCRE regex patterns work: https://github.com/JuliaLang/julia/blob/master/base/regex.jl#L10. Not sure if that's what's going on here though.

Member

StefanKarpinski commented Apr 11, 2013

I'm confused: why is MPFRFloat.mpfr a Vector{Int32}, instead of being an opaque mpfr_t pointer?

I don't know if it's the case here, but in cases where a fixed amount of memory is used to store some C blob, you can make the thing serializable by using a Julia array for the storage. E.g. that's how compiled PCRE regex patterns work: https://github.com/JuliaLang/julia/blob/master/base/regex.jl#L10. Not sure if that's what's going on here though.

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

@StefanKarpinski, that doesn't seem to be what's going on here. mpfr_t in C is an array (pointer) of one __mpfr_struct, which consists of three integers plus a pointer. MPFR.jl uses a Vector{Int32} of length 5 instead, where I guess 5 comes from the number of Int32s you need to store 3 32-bit int variables and a 64-bit pointer?? One problem is that 5 seems wrong to me: with struct padding, it seems like more space is required on a 64-bit architecture, and moreover one of the 3 integer fields is a long which is 64 bits on 64-bit Unix systems. In any case, the naive serialize function won't work for this because you can't just serialize the value of the pointer, you have to serialize the contents of the pointed-to array. So, it needs its own serialize method anyway.

The problem here is that MPFR's interface, now that I look at it, requires the caller to know the layout (or at least the size) of the __mpfr_struct, since it doesn't provide a way to allocate an opaque mpfr_t pointer.

There are two possibilities that occur to me. One is to just make BigFloat a type with some padding that is at least as big as __mpfr_struct, i.e. type BigFloat; pad0::Int32; pad1::Int32; .......; end. The other (better, I think) is to try and match the actual size by copying the original struct definition from mpfr.h

type BigFloat
     prec::Cint # problem: may be a Cshort in some MPFR compilations
     sign::Cint
     exp::Clong
     d::Ptr{Void} # pointer to mp_limb_t, which can vary in size depending on how GMP is compiled
end

However, since this will be part of the Julia build, it should be straightforward to extract the correct types from the header files to insert into the .jl file (e.g. you can run a little C program to print the sizeof values, and there are even tricks to get sizeof that only require running the compiler so that it works when cross-compiling).

(It shouldn't be immutable since the contents are, in fact, mutable by the C API.) This would avoid the double allocation (where you now allocate a type whose contents are a pointer to an array that contains another struct). Then you would pass a Ptr{BigFloat} (via &) to MPFR functions and it should work, assuming the struct types match. Getting Complex{BigFloat} to work with MPC is a more tricky, however, since I don't think Complex{BigFloat} will then have the same layout as a struct of two __mpfr_structs. (It would if BigFloat were immutable, but BigFloat must be mutable.)

Member

stevengj commented Apr 11, 2013

@StefanKarpinski, that doesn't seem to be what's going on here. mpfr_t in C is an array (pointer) of one __mpfr_struct, which consists of three integers plus a pointer. MPFR.jl uses a Vector{Int32} of length 5 instead, where I guess 5 comes from the number of Int32s you need to store 3 32-bit int variables and a 64-bit pointer?? One problem is that 5 seems wrong to me: with struct padding, it seems like more space is required on a 64-bit architecture, and moreover one of the 3 integer fields is a long which is 64 bits on 64-bit Unix systems. In any case, the naive serialize function won't work for this because you can't just serialize the value of the pointer, you have to serialize the contents of the pointed-to array. So, it needs its own serialize method anyway.

The problem here is that MPFR's interface, now that I look at it, requires the caller to know the layout (or at least the size) of the __mpfr_struct, since it doesn't provide a way to allocate an opaque mpfr_t pointer.

There are two possibilities that occur to me. One is to just make BigFloat a type with some padding that is at least as big as __mpfr_struct, i.e. type BigFloat; pad0::Int32; pad1::Int32; .......; end. The other (better, I think) is to try and match the actual size by copying the original struct definition from mpfr.h

type BigFloat
     prec::Cint # problem: may be a Cshort in some MPFR compilations
     sign::Cint
     exp::Clong
     d::Ptr{Void} # pointer to mp_limb_t, which can vary in size depending on how GMP is compiled
end

However, since this will be part of the Julia build, it should be straightforward to extract the correct types from the header files to insert into the .jl file (e.g. you can run a little C program to print the sizeof values, and there are even tricks to get sizeof that only require running the compiler so that it works when cross-compiling).

(It shouldn't be immutable since the contents are, in fact, mutable by the C API.) This would avoid the double allocation (where you now allocate a type whose contents are a pointer to an array that contains another struct). Then you would pass a Ptr{BigFloat} (via &) to MPFR functions and it should work, assuming the struct types match. Getting Complex{BigFloat} to work with MPC is a more tricky, however, since I don't think Complex{BigFloat} will then have the same layout as a struct of two __mpfr_structs. (It would if BigFloat were immutable, but BigFloat must be mutable.)

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

Also, it's not completely clear to me that we need MPC. If we just do Complex{BigFloat}, the generic Complex{T} methods seem like they provide pretty much all of what MPC provides.

(There doesn't seem to be anything too special in the MPC implementations; they just convert everything into the corresponding operations on the real and imaginary parts. They might be more clever about minimizing the number of temporary values compared to Complex{T}. It would be interesting to have a benchmark or two.)

Member

stevengj commented Apr 11, 2013

Also, it's not completely clear to me that we need MPC. If we just do Complex{BigFloat}, the generic Complex{T} methods seem like they provide pretty much all of what MPC provides.

(There doesn't seem to be anything too special in the MPC implementations; they just convert everything into the corresponding operations on the real and imaginary parts. They might be more clever about minimizing the number of temporary values compared to Complex{T}. It would be interesting to have a benchmark or two.)

@ViralBShah

This comment has been minimized.

Show comment Hide comment
@ViralBShah

ViralBShah Apr 11, 2013

Member

@stevengj That is useful to know. In that case, let's get MPFR in shape. It would be nice to leverage Complex{BigFloat} as much as possible and avoid adding another library.

Member

ViralBShah commented Apr 11, 2013

@stevengj That is useful to know. In that case, let's get MPFR in shape. It would be nice to leverage Complex{BigFloat} as much as possible and avoid adding another library.

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

PS. It looks like you need to wrap the mpfr_hypot function. (This is necessary for abs(::Complex{BigFloat}) to work.)

Member

stevengj commented Apr 11, 2013

PS. It looks like you need to wrap the mpfr_hypot function. (This is necessary for abs(::Complex{BigFloat}) to work.)

@staticfloat

This comment has been minimized.

Show comment Hide comment
@staticfloat

staticfloat Apr 11, 2013

Member

Everything looks good. Once this lands, we can make some very simple changes to .travis.yml. No packaging required on my end! :D

I suppose we could also include the travis changes in this pull request..... that might actually be the best plan, as then we get to test the travis integration with the pull request itself.

Member

staticfloat commented Apr 11, 2013

Everything looks good. Once this lands, we can make some very simple changes to .travis.yml. No packaging required on my end! :D

I suppose we could also include the travis changes in this pull request..... that might actually be the best plan, as then we get to test the travis integration with the pull request itself.

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

Another TODO item: BigFloat really shouldn't use promotion rules to perform e.g. *(::BigFloat,::Float64), since converting the second argument to a BigFloat before multiplying is heinously inefficient compared to calling mpfr_mul_d.

This is also a flaw of our BigInt implementation, by the way, since GMP provides special functions to add a BigInt to a Clong etcetera that we aren't exploiting (see #2839).

Member

stevengj commented Apr 11, 2013

Another TODO item: BigFloat really shouldn't use promotion rules to perform e.g. *(::BigFloat,::Float64), since converting the second argument to a BigFloat before multiplying is heinously inefficient compared to calling mpfr_mul_d.

This is also a flaw of our BigInt implementation, by the way, since GMP provides special functions to add a BigInt to a Clong etcetera that we aren't exploiting (see #2839).

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

I notice that the BigInt implementation in bigint.jl pulls the same ugly stunt: it allocates an Array(Int32, 4) to hold an __mpz_struct, which should really be a

type BigInt
    alloc::Cint
    size::Cint
    d::Ptr{Void}
 end

to match the C struct and avoid the double allocation. Again, I think it should be mutable (since the entries are mutable in the C API).

Member

stevengj commented Apr 11, 2013

I notice that the BigInt implementation in bigint.jl pulls the same ugly stunt: it allocates an Array(Int32, 4) to hold an __mpz_struct, which should really be a

type BigInt
    alloc::Cint
    size::Cint
    d::Ptr{Void}
 end

to match the C struct and avoid the double allocation. Again, I think it should be mutable (since the entries are mutable in the C API).

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 11, 2013

Member

I wrote a comment here, but you all basically said everything I was going to say and more, so thanks for the help.

I was trying to make the suggestion work with BigInt as preliminary test, but I got a bunch of stack overflows randomly, and then I wasn't able to compile Julia anymore because it wasn't able to read libreadline.a and some other files, so I'm doing a fresh install of Julia and updating gcc/glibc on this computer to work on it again.

Member

andrioni commented Apr 11, 2013

I wrote a comment here, but you all basically said everything I was going to say and more, so thanks for the help.

I was trying to make the suggestion work with BigInt as preliminary test, but I got a bunch of stack overflows randomly, and then I wasn't able to compile Julia anymore because it wasn't able to read libreadline.a and some other files, so I'm doing a fresh install of Julia and updating gcc/glibc on this computer to work on it again.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 11, 2013

Member

@staticfloat @ViralBShah the build I did for this pull request is already with a modified .travis.yml to use the system MPFR and MPC (they are needed for newer GCC versions, so they are bundled with it in almost every Linux distro)

Member

andrioni commented Apr 11, 2013

@staticfloat @ViralBShah the build I did for this pull request is already with a modified .travis.yml to use the system MPFR and MPC (they are needed for newer GCC versions, so they are bundled with it in almost every Linux distro)

@staticfloat

This comment has been minimized.

Show comment Hide comment
@staticfloat

staticfloat Apr 11, 2013

Member

hahaha, so I ended up putting USE_SYSTEM_MPFR in there twice! Awesome. Nice work, @andrioni. I would only ask you to include libmpfr-dev in the apt-get install line, as many users use .travis.yml as a template for an easy Ubuntu install, and if they don't have mpfr, they will need that line too.

Member

staticfloat commented Apr 11, 2013

hahaha, so I ended up putting USE_SYSTEM_MPFR in there twice! Awesome. Nice work, @andrioni. I would only ask you to include libmpfr-dev in the apt-get install line, as many users use .travis.yml as a template for an easy Ubuntu install, and if they don't have mpfr, they will need that line too.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 11, 2013

Member

BigInt now uses the approach proposed by @stevengj, and the .travis.yml now installs the dependencies correctly.

Member

andrioni commented Apr 11, 2013

BigInt now uses the approach proposed by @stevengj, and the .travis.yml now installs the dependencies correctly.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 11, 2013

Member

__GMP_MP_SIZE_T_INT only gets set on some Cray architectures, so _MPFR_PREC_FORMAT gets set to 3 and so both the default precision and the exponent variables are long, if my interpretation of mpfr.h is right.

(by the way, kudos to the one who implemented Cint and Clong types.)

Member

andrioni commented Apr 11, 2013

__GMP_MP_SIZE_T_INT only gets set on some Cray architectures, so _MPFR_PREC_FORMAT gets set to 3 and so both the default precision and the exponent variables are long, if my interpretation of mpfr.h is right.

(by the way, kudos to the one who implemented Cint and Clong types.)

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 11, 2013

Member

pointer_from_objref won't work because it returns a jl_value_t*, which has some header information before the member data starts. @vtjnash, will #2818 fix this (so that pointer_from_objref(x::Foo) returns a pointer that can be passed as a C struct corresponding to Foo)?

Member

stevengj commented Apr 11, 2013

pointer_from_objref won't work because it returns a jl_value_t*, which has some header information before the member data starts. @vtjnash, will #2818 fix this (so that pointer_from_objref(x::Foo) returns a pointer that can be passed as a C struct corresponding to Foo)?

@vtjnash

This comment has been minimized.

Show comment Hide comment
@vtjnash

vtjnash Apr 12, 2013

Member

Actually the purpose of #2818 is to help eliminate uses of pointer_from_objref. In this case, it would make the following work: ptrarr = [x.mpfr for x in arr] (although the previous suggestion would also be made to work, but be more difficult from a garbage collection and usage standpoint)

Member

vtjnash commented Apr 12, 2013

Actually the purpose of #2818 is to help eliminate uses of pointer_from_objref. In this case, it would make the following work: ptrarr = [x.mpfr for x in arr] (although the previous suggestion would also be made to work, but be more difficult from a garbage collection and usage standpoint)

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 12, 2013

Member

@andrioni, note that you shouldn't need to have a separate mpfr_struct type. Just put the members directly in BigFloat:

type BigFloat{N} <: FloatingPoint
    prec::Clong
    sign::Cint
    exp::Clong
    d::Ptr{Void}
end

Similarly, BigInt shouldn't need a separate mpz_struct.

Member

stevengj commented Apr 12, 2013

@andrioni, note that you shouldn't need to have a separate mpfr_struct type. Just put the members directly in BigFloat:

type BigFloat{N} <: FloatingPoint
    prec::Clong
    sign::Cint
    exp::Clong
    d::Ptr{Void}
end

Similarly, BigInt shouldn't need a separate mpz_struct.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 12, 2013

Member

I was having some issues with constructors before, but then again, it seems a recompile after make distclean solved all my stack overflows. I'm updating it again right now.

Member

andrioni commented Apr 12, 2013

I was having some issues with constructors before, but then again, it seems a recompile after make distclean solved all my stack overflows. I'm updating it again right now.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 13, 2013

Member
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00007fff5f3ffff8
0x0000000101f5dc48 in ?? ()

Ok, I got more stack overflows. Here's the code I tried to use. I'll keep improving this using the auxiliary mp*_struct types, and when I find a solution for these stack overflows (basically, the constructor seems to call itself endlessly if you define the type and some functions for it, but not if there aren't extra functions).

Member

andrioni commented Apr 13, 2013

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00007fff5f3ffff8
0x0000000101f5dc48 in ?? ()

Ok, I got more stack overflows. Here's the code I tried to use. I'll keep improving this using the auxiliary mp*_struct types, and when I find a solution for these stack overflows (basically, the constructor seems to call itself endlessly if you define the type and some functions for it, but not if there aren't extra functions).

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 13, 2013

Member

@JeffBezanson, do you see why this version of the BigInt() constructor would lead to a stack overflow?

Member

stevengj commented Apr 13, 2013

@JeffBezanson, do you see why this version of the BigInt() constructor would lead to a stack overflow?

@StefanKarpinski

This comment has been minimized.

Show comment Hide comment
@StefanKarpinski

StefanKarpinski Apr 13, 2013

Member

Passing &b to gmpz_init where b is the Julia BigInt object seems like a bad idea. That pointer is going to be a jl_value_t* and include the box (type tag, etc.) in front of the actual value. I'm not sure what the best way to do this is, but you need to get a pointer to the data of the value, not the value itself. Could be as simple as adding 1 to the pointer, but that feels kind of hacky. Also, @vtjnash's "recurse types" branch might change this by making Julia reference point at the beginning of the data of Julia values instead of the box.

Member

StefanKarpinski commented Apr 13, 2013

Passing &b to gmpz_init where b is the Julia BigInt object seems like a bad idea. That pointer is going to be a jl_value_t* and include the box (type tag, etc.) in front of the actual value. I'm not sure what the best way to do this is, but you need to get a pointer to the data of the value, not the value itself. Could be as simple as adding 1 to the pointer, but that feels kind of hacky. Also, @vtjnash's "recurse types" branch might change this by making Julia reference point at the beginning of the data of Julia values instead of the box.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 13, 2013

Member

Why it works then when I use the auxiliary mpz_struct, shouldn't it be boxed as well?
(I had a similar problem for the mpc_struct, as using two mpfr_struct as members directly doesn't work, but hand 'unrolling' them (i.e. putting the mpfr_struct fields directly, as in f17210d242597cc64b1037776a5318e15d0647ee) works fine.)

Member

andrioni commented Apr 13, 2013

Why it works then when I use the auxiliary mpz_struct, shouldn't it be boxed as well?
(I had a similar problem for the mpc_struct, as using two mpfr_struct as members directly doesn't work, but hand 'unrolling' them (i.e. putting the mpfr_struct fields directly, as in f17210d242597cc64b1037776a5318e15d0647ee) works fine.)

@StefanKarpinski

This comment has been minimized.

Show comment Hide comment
@StefanKarpinski

StefanKarpinski Apr 13, 2013

Member

If that works, then maybe I'm wrong about this. The & syntax may provide a pointer to the data rather than the box. In that case the issue must be something else.

Member

StefanKarpinski commented Apr 13, 2013

If that works, then maybe I'm wrong about this. The & syntax may provide a pointer to the data rather than the box. In that case the issue must be something else.

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 13, 2013

Member

@StefanKarpinski, my understanding is that &b is not a jl_value_t*, it is the pointer to the member data. This came up on the julia-dev list when I was developing PyCall. This is necessary in order to call C routines expecting struct pointers, and has worked for a long time.

#2818 goes further than this, and makes jl_value_t* point to the member data, which erases the current distinction between pointer_from_objref(b) and &b.

Member

stevengj commented Apr 13, 2013

@StefanKarpinski, my understanding is that &b is not a jl_value_t*, it is the pointer to the member data. This came up on the julia-dev list when I was developing PyCall. This is necessary in order to call C routines expecting struct pointers, and has worked for a long time.

#2818 goes further than this, and makes jl_value_t* point to the member data, which erases the current distinction between pointer_from_objref(b) and &b.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 14, 2013

Member

If so, then it makes sense why this works

type mpc_struct
    # Real MPFR part
    reprec::Clong
    resign::Cint
    reexp::Clong
    red::Ptr{Void}
    # Imaginary MPFR part
    imprec::Clong
    imsign::Cint
    imexp::Clong
    imd::Ptr{Void}
end

while this doesn't

type mpfr_struct
    prec::Clong
    sign::Cint
    exp::Clong
    d::Ptr{Void}
end
type mpc_struct
    re::mpfr_struct
    im::mpfr_struct
end

The second one probably includes the boxes for the mpfr_structs, so it segfaults when I call mpc_init3.

Member

andrioni commented Apr 14, 2013

If so, then it makes sense why this works

type mpc_struct
    # Real MPFR part
    reprec::Clong
    resign::Cint
    reexp::Clong
    red::Ptr{Void}
    # Imaginary MPFR part
    imprec::Clong
    imsign::Cint
    imexp::Clong
    imd::Ptr{Void}
end

while this doesn't

type mpfr_struct
    prec::Clong
    sign::Cint
    exp::Clong
    d::Ptr{Void}
end
type mpc_struct
    re::mpfr_struct
    im::mpfr_struct
end

The second one probably includes the boxes for the mpfr_structs, so it segfaults when I call mpc_init3.

@vtjnash

This comment has been minimized.

Show comment Hide comment
@vtjnash

vtjnash Apr 14, 2013

Member

Those two declarations are quite different in that the mpfr_struct is allocated inline in the mpc_struct in the first example, but in the second example, mpc_struct merely contains two pointers (jl_value_t*). My work in #2818 would make the 2nd example compatible with many C libraries, but not this one.

Member

vtjnash commented Apr 14, 2013

Those two declarations are quite different in that the mpfr_struct is allocated inline in the mpc_struct in the first example, but in the second example, mpc_struct merely contains two pointers (jl_value_t*). My work in #2818 would make the 2nd example compatible with many C libraries, but not this one.

@stevengj

This comment has been minimized.

Show comment Hide comment
@stevengj

stevengj Apr 14, 2013

Member

See above: I'm still not clear on why we need MPC at all. What advantage does it bring over Complex{BigFloat}, assuming BigFloat is implemented with MPFR?

Member

stevengj commented Apr 14, 2013

See above: I'm still not clear on why we need MPC at all. What advantage does it bring over Complex{BigFloat}, assuming BigFloat is implemented with MPFR?

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 15, 2013

Member

Potentially more efficient implementations of the trigonometric functions (I have yet to check) and having different precisions for the real and the imaginary parts. MPACK (multiple precision BLAS+LAPACK) may use MPC, but I don't if it's needed or not.

Member

andrioni commented Apr 15, 2013

Potentially more efficient implementations of the trigonometric functions (I have yet to check) and having different precisions for the real and the imaginary parts. MPACK (multiple precision BLAS+LAPACK) may use MPC, but I don't if it's needed or not.

@ViralBShah

This comment has been minimized.

Show comment Hide comment
@ViralBShah

ViralBShah Apr 16, 2013

Member

We do not use libm implementations for the complex trig functions for single and double precision. It would be better to avoid an extra library in the build, if possible, and use generic julia code. Of course, we can always add MPC later, if we change our mind.

Member

ViralBShah commented Apr 16, 2013

We do not use libm implementations for the complex trig functions for single and double precision. It would be better to avoid an extra library in the build, if possible, and use generic julia code. Of course, we can always add MPC later, if we change our mind.

@JeffBezanson

This comment has been minimized.

Show comment Hide comment
@JeffBezanson

JeffBezanson Apr 17, 2013

Member

I'm going to cherry pick the changes to BigInt that close #2839.

Member

JeffBezanson commented Apr 17, 2013

I'm going to cherry pick the changes to BigInt that close #2839.

@JeffBezanson

This comment has been minimized.

Show comment Hide comment
@JeffBezanson

JeffBezanson Apr 17, 2013

Member

Ok, why is there both mpz_struct and BigInt wrapping it? Couldn't we make do with just the former?

Member

JeffBezanson commented Apr 17, 2013

Ok, why is there both mpz_struct and BigInt wrapping it? Couldn't we make do with just the former?

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni Apr 17, 2013

Member

Using BigInt directly instead of having mpz_struct leads to stack overflows. An example of BigInt in this way is here.

Member

andrioni commented Apr 17, 2013

Using BigInt directly instead of having mpz_struct leads to stack overflows. An example of BigInt in this way is here.

@JeffBezanson

This comment has been minimized.

Show comment Hide comment
@JeffBezanson

JeffBezanson Apr 17, 2013

Member

Of course. Damn there is too much going on and I'm not able to keep track of everything.

Member

JeffBezanson commented Apr 17, 2013

Of course. Damn there is too much going on and I'm not able to keep track of everything.

andrioni added some commits Apr 11, 2013

Update MPFRFloat to use a precise sized mpfr_struct
As discussed in #2814. The function `sum` is disabled currently
(so the default one is used as a fallback), as it needs direct
pointer access to each of the members of the array.
Rename precision-related functions and add them to Float32 and Float64
Now get_precision is defined for all T <: FloatingPoint, and returns
24 for Float32 and 53 for Float64 (the effective number of bits in the
mantissa, taking in account the implicit leading bit.

get_bigfloat_precision and set_bigfloat_precision are used to get and set
the defaults for all BigFloats, and the BigComplex case is analogous.
Update MPCComplex to use a precise mpc_struct
As using two mpfr_structs doesn't work, I decided to expand them
directly into two groups of four elements (two longs, an int and
a pointer). They could be useful if we ever need to implement
efficient in-place algorithms, as they can be directly extracted
and manipulated as MPFRFloats.
Convert methods for other common complex types available
These methods were written in order to avoid having to use real()
or imag(), as they create a new MPFRFloat object.
Use more efficient addition/subtraction/multiplication for ints/uints
This also is the first function which tries to take in account
that Clong is not always Int.
Fix ccall type issues in bigint.jl, according to #2901
Now it uses Culong and Clong when needed.
Implement efficient basic arithmetic for signed/unsigned/float/BigInt…
… and MPFRFloat

The four basic operations are now implemented using the MPFR functions
instead of promotion to MPFRFloat.
Add exp, log, trigonometric functions and @simonbyrne's test suite
Some sign issues were found and commented in test file, and no
errors are thrown yet.
Add ldexp and atan2 functions to MPFRFloat
As they are being used by @jiahao in the new Kahan complex functions,
it is needed to have them in MPFRFloat.
Remove all mentions to MPC, as it was decided to scrap it
Now BigComplex support is given directly through Complex{BigFloat}.
@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni May 3, 2013

Member

Can someone comment on the rounding modes? Currently I'm using constants in the MPFR module, should I rename/export them?

Other than that, I think it is finally reasonably finished. I've also rebased it (up to 9a72fab), so the merge should be simple.

Member

andrioni commented May 3, 2013

Can someone comment on the rounding modes? Currently I'm using constants in the MPFR module, should I rename/export them?

Other than that, I think it is finally reasonably finished. I've also rebased it (up to 9a72fab), so the merge should be simple.

@StefanKarpinski

This comment has been minimized.

Show comment Hide comment
@StefanKarpinski

StefanKarpinski May 3, 2013

Member

See #2976 – we don't currently expose rounding modes, but we should have a uniform interface across IEEE 754 and other types of floats for rounding modes. So I would say hold off exposing if for now and leave it as part of #2976.

Member

StefanKarpinski commented May 3, 2013

See #2976 – we don't currently expose rounding modes, but we should have a uniform interface across IEEE 754 and other types of floats for rounding modes. So I would say hold off exposing if for now and leave it as part of #2976.

@JeffBezanson

This comment has been minimized.

Show comment Hide comment
@JeffBezanson

JeffBezanson May 3, 2013

Member

Why are we now back to using separate BigInt and mpz_struct types? Not a big deal, I guess I can change it myself.
Also, unfortunately there is a new merge conflict.

Member

JeffBezanson commented May 3, 2013

Why are we now back to using separate BigInt and mpz_struct types? Not a big deal, I guess I can change it myself.
Also, unfortunately there is a new merge conflict.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni May 3, 2013

Member

Ahem, I unfortunately destroyed the changes on the repo trying to merge it on my lab computer, I'll restore it as soon as I get back home.

Member

andrioni commented May 3, 2013

Ahem, I unfortunately destroyed the changes on the repo trying to merge it on my lab computer, I'll restore it as soon as I get back home.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni May 3, 2013

Member

Which should be in twenty minutes.

Member

andrioni commented May 3, 2013

Which should be in twenty minutes.

@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni May 3, 2013

Member

Restored now. (that's why one should never use rebase to rewrite history nor try to manually push changes on a Friday afternoon :))

Member

andrioni commented May 3, 2013

Restored now. (that's why one should never use rebase to rewrite history nor try to manually push changes on a Friday afternoon :))

@JeffBezanson

This comment has been minimized.

Show comment Hide comment
@JeffBezanson

JeffBezanson May 3, 2013

Member

Awesome! Hoping to merge this today.

Member

JeffBezanson commented May 3, 2013

Awesome! Hoping to merge this today.

JeffBezanson added a commit that referenced this pull request May 3, 2013

@JeffBezanson JeffBezanson merged commit fd89714 into JuliaLang:master May 3, 2013

1 check passed

default The Travis build passed
Details
@andrioni

This comment has been minimized.

Show comment Hide comment
@andrioni

andrioni May 3, 2013

Member

Feel free to assign me to any issues related to BigFloat or BigInt, I'm willing to support them and help as much as I can :)

Member

andrioni commented May 3, 2013

Feel free to assign me to any issues related to BigFloat or BigInt, I'm willing to support them and help as much as I can :)

@JeffBezanson

This comment has been minimized.

Show comment Hide comment
@JeffBezanson

JeffBezanson May 3, 2013

Member

Thanks, and thanks again for doing this. After a couple small tweaks it's looking good!

Member

JeffBezanson commented May 3, 2013

Thanks, and thanks again for doing this. After a couple small tweaks it's looking good!

@ViralBShah

This comment has been minimized.

Show comment Hide comment
@ViralBShah

ViralBShah May 4, 2013

Member

@andrioni It would be great if you could add the documentation for BigFloat and even review the BigInt documentation. For the rest of the issues that are not addressed here (make check, sum, etc.), I think we should open new issues so that these are not lost.

Fantastic work.

Member

ViralBShah commented May 4, 2013

@andrioni It would be great if you could add the documentation for BigFloat and even review the BigInt documentation. For the rest of the issues that are not addressed here (make check, sum, etc.), I think we should open new issues so that these are not lost.

Fantastic work.

@andrioni andrioni referenced this pull request May 7, 2013

Closed

0.2 release notes #2581

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment