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

rename Complex{32,64,128} to ComplexF{16,32,64} #24647

Merged
merged 1 commit into from
Dec 13, 2017
Merged

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Nov 18, 2017

Cf. https://discourse.julialang.org/t/rename-complex2n-to-complexn/6081 for background.
Here is a recollection from this thread of 3 reasons to deprecate the ComplexN aliases:

  • In Complex128, 128 refers to no semantic reality connected to the type. It only refers to the very low-level fact that it's its number of bits. Actually, in the initial discourse thread, I suggested renaming Complex128 to Complex64 with the idea "the coordinates of a Complex64 is Float64", so there is a clearer connection between the 2 types.
  • Complex128 is ambiguous, since any eltype with 64 bit size (e.g. Int64) will give you a Complex type of size 128 (@Keno);
  • Historically, the Complex128 type actually preceded Complex{Float64} — it was originally a bits type because Julia didn’t have immutable types. So, at this point, deprecating Complex128 makes a lot of sense (@stevengj).

@rfourquet rfourquet added the kind:deprecation This change introduces or involves a deprecation label Nov 18, 2017
@@ -65,6 +65,9 @@ const liblapack = Base.liblapack_name

import ..LinAlg: BlasReal, BlasComplex, BlasFloat, BlasInt, DimensionMismatch, checksquare, axpy!

const Complex64 = Complex{Float32}
const Complex128 = Complex{Float64}
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused. Isn't the point to remove definitions like this?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The BLAS module can define whatever convenience typealiases it wants though, right?

Copy link
Member

Choose a reason for hiding this comment

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

Well sure, but it seems like for clarity it might be nice to use the more explicit type names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, one reason I did that is that this file and in particular "lapack.jl" contain an awful lot of references to Complex128/Complex64, so I thougth it was fine to have local aliases (which I would certainly do for a personal project). Morever, I am really not familiar with the LinAlg module, so I prefer to receive suggestions from its authors as to what they prefer (alias or not). Also this module will eventually be split out from base, and so will these aliases.

@alyst
Copy link
Contributor

alyst commented Nov 18, 2017

I suggested renaming Complex128 to Complex64

Is it the plan to deprecate the current ComplexN aliases and then to reintroduce them as ComplexN/2?

IIUC Complex{Float64} is by far the most commonly used complex type. It would be a pity not to have a shorter alias for it.

@ararslan ararslan added the domain:complex Complex numbers label Nov 18, 2017
@rfourquet
Copy link
Member Author

Is it the plan to deprecate the current ComplexN aliases and then to reintroduce them as ComplexN/2?

It would be my preference if there has to be aliases, but according to the discourse thread, it doesn't seem to considered as a good idea. But of course more opinion can be expressed and I really don't know at this point what will be decided.

@@ -15,6 +15,8 @@ struct MatrixIllConditionedException <: Exception
msg::AbstractString
end

const Complex128 = Complex{Float64}
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

@@ -14,6 +14,9 @@ import ..LinAlg: BlasFloat, Char, BlasInt, LAPACKException,

using Base: iszero

const Complex64 = Complex{Float32}
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of these as well?

Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine either way with the local aliases in the linalg code. We could also teach femtocleaner to do this rewrite and have it be done automatically.

@rfourquet
Copy link
Member Author

could also teach femtocleaner to do this rewrite and have it be done automatically.

This could indeed be more safe. One of the other reason I used these local aliase is that ComplexN is used a lot as a Symbol which is interpolated, so the transformation would generally be e.g. :Complex128 -> Complex{Float64}, which sed can do easily but this would need careful review. But again, I'm fine to do whatever linalg people tell me to do (if this PR is triage'd positively).

@antoine-levitt
Copy link
Contributor

I use things like zeros(Complex128, n, n) a lot. Zeros(Complex{Float64}, n, n) is not as nice. That and the other array builder functions are the main reason I can think of where having the aliases is very convenient. It's not that I want to have complex types that are exactly 128 bits wide, I just want to build an array of complex floating point numbers, in whatever floating point type happens to be the default (which turns out to be Float64). The fact that I even have to know that Complex128 is the right type for that is in itself a design flaw from the perspective of a Matlab-like language (although of course it makes sense for julia). Would overloading zeros(Complex, dims) to be zeros(Complex{Float64}, dims) be feasible / a good idea?

As far as renaming to complex64, I think that would be a good idea in theory but it's the opposite convention from numpy, which is unnecessarily annoying.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Nov 18, 2017

Consider using

julia> typeof(fill(0.0+0.0im, 5, 5))
Array{Complex{Float64},2}

julia> typeof(fill(0.f0+0.f0im, 5, 5))
Array{Complex{Float32},2}

or something of that sort.

@Sacha0
Copy link
Member

Sacha0 commented Nov 18, 2017

Even simpler: fill(0.0im, n, n) :).

@JeffBezanson
Copy link
Sponsor Member

I'm not sure how I feel about this. I see that the names are a bit redundant and confusing, but it also seems to me that if the aliases are useful for the linalg code then they're useful for others as well.

@Sacha0
Copy link
Member

Sacha0 commented Nov 18, 2017

I see that the names are a bit redundant and confusing, but it also seems to me that if the aliases are useful for the linalg code then they're useful for others as well.

I favor removing those aliases from linalg and sparse. (Edit: Those aliases usually confuse me when reading the linalg and sparse code, and the small amount of additional typing that the explicit names require seems worth the improvement in clarity.) Best!

@rfourquet
Copy link
Member Author

if the aliases are useful for the linalg code then they're useful for others as well.

From what i could see, those types are used in linalg mainly for wrapping libraries, with loops and eval, so this is mainly to be exhaustive, but using Complex{Float64} instead of Complex128 would'n hurt clarity I think, as these appeary only as variables of those loops. So actually I would favor also getting rid entirely of the aliases even there. I will do that tomorrow as there seem to be support for this.

@andyferris
Copy link
Member

Other languages like maybe Go have these type names also, no? (Mostly, I would avoid the rename to ComplexN/2 since it seems nonstandard across languages; I don’t tend to use Complex128 myself).

@rfourquet rfourquet added the needs decision A decision on this change is needed label Nov 25, 2017
@rfourquet
Copy link
Member Author

Added the "decision" label. There is support here and on the discourse thread, but I'm not the one to make the final call. As this is breaking, I will resort to "triage" eventually if needed.

@StefanKarpinski StefanKarpinski added the status:triage This should be discussed on a triage call label Nov 27, 2017
@StefanKarpinski
Copy link
Sponsor Member

No need to be that shy with the triage label, that's what it's for. We may as well discuss it.

@StefanKarpinski
Copy link
Sponsor Member

NumPy and Go use analogous names. Rust uses Complex64 to mean Complex{Float64}.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Nov 30, 2017

I'm fine with deprecating these types. We may want to introduce ComplexF64 and ComplexF128 at some point as shorthands for Complex{Float64} and Complex{Float128}, but that can be done any any point, so let's wait on that.

@StefanKarpinski StefanKarpinski removed the status:triage This should be discussed on a triage call label Nov 30, 2017
@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Nov 30, 2017
@garrison
Copy link
Sponsor Member

I've been thinking more about why I am opposed to this. My main objection is about Complex128; the others are far less useful to me. At the moment, I can write zeros(N) to get a Vector of Float64s, but if I want a vector of equivalent complex numbers I write zeros(Complex128, N). With this change, complex numbers feel even more like second class citizens, as the expression becomes zeros(Complex{Float64}, N).

I think it is telling that the original pull request left in these aliases for use in the linear algebra code. These aliases are useful and will be even more so in user code, especially for those of us using complex numbers frequently.

One thing I especially like about the status quo -- and this gets to the heart of my thumbs down -- is that the aliases are standardized. I would hate to see various code bases define these aliases on their own. Right now, if I see Complex64 in julia code, I know precisely what it means; I do not need to wonder whether the local code base defined it to be Complex{Float64} or Complex{Float32}. I think @StefanKarpinski's suggested ComplexF... names can help with this in the long run, if people can all agree on the name. If the path forward is indeed to have aliases that are more clearly named, I would remove my objection completely.

As for ambiguity, it is already possible to learn what Complex128 means by simply typing it into the repl:

julia> Complex128
Complex{Float64}

Best! [channeling @Sacha0 here... 😉]

@Sacha0
Copy link
Member

Sacha0 commented Dec 1, 2017

Regarding zeros(Complex128, N), why not fill(0.0im, N)? The latter is both shorter and less ambiguous :).

(Edit: Oh gosh, I forgot the...) Best!

@rfourquet
Copy link
Member Author

If the path forward is indeed to have aliases that are more clearly named, I would remove my objection completely.

I agree that it could be better to still have "official" aliases. I'm not a big user of Complex types myself, so I always have a (very) small moment of confusion when I deal with them, in that the translation from the 128 suffix to Float64 is not automatic yet in my brain. In my original post on discourse (linked to in the OP), I was suggesting to rename Complex128 to Complex64, which turned out to be a bad idea, but ComplexF64 would be fine for me; and I was arguing that it's not scalable to name a type after it's total number of bits: I would start to get really confused with Quaternion256 or Octonion512 ;-p

So I understand that the removal of triage is a green light to go on with this, so I will rebase withing the next few days, and I volunteer to introduce the ComplexF64 aliases if it's decided upon.

@garrison
Copy link
Sponsor Member

garrison commented Dec 1, 2017

why not fill(0.0im, N)?

On first glance this looks to me like initializing a purely imaginary number rather than a complex number. Of course, julia does not have purely imaginary numbers, so it indeed results in a complex number after an additional mental step. I believe I could get accustomed to this for initializing zeroed arrays.

Aside from initializing arrays, I often type Complex128 in places where I suspect inference may not succeed, such as to specify the type of a comprehension explicitly. My understanding is that inference is not meant to be predictable in general, but I do not have a sophisticated understanding of when (if ever) I should expect it to be predictable, which results in me specifying the type often. I have run into numerous instances where type inference fails in places where I would expect it to be obvious by inspection (thus slowing my code down significantly). For instance, the code I reported at #17717 (comment) behaves in recent versions as I would expect, but I can come up with slightly more complicated examples where inference still unexpectedly fails with comprehensions. I am not sure if this is inherent or not (helping me understand this, of course, is a topic better suited for Discourse 😉), but if it is indeed the case that many simple type-inference cases should not be predictable, then I believe the aliases are a lot more useful than they might otherwise be.

Best!

@Sacha0
Copy link
Member

Sacha0 commented Dec 1, 2017

On first glance this looks to me like initializing a purely imaginary number rather than a complex number. Of course, julia does not have purely imaginary numbers, so it indeed results in a complex number after an additional mental step. I believe I could get accustomed to this for initializing zeroed arrays.

Perhaps fill(0.0 + 0.0im, N) if you prefer? :)

Edit: Or even fill(Complex(0.0), N), if that better suits your style? :)

@rfourquet rfourquet force-pushed the rf/complex128 branch 2 times, most recently from 6285cbb to 06f22ba Compare December 8, 2017 11:55
@rfourquet
Copy link
Member Author

Triage decided to introduce new aliases, namely ComplexF64 instead of Complex128 etc.
I updated the PR to reflect that, by applying sed mainly (except "NEWS.md" and "deprecated.jl"), so it's less likely to contain human mistakes, but also it's possible one instance or two have been replaced when they shouldn't!

@rfourquet rfourquet changed the title deprecate Complex{32,64,128} rename Complex{32,64,128} to ComplexF{16,32,64} Dec 8, 2017
@rfourquet rfourquet removed the needs decision A decision on this change is needed label Dec 8, 2017
xtyp(::Type{Complex64}) = COMPLEX
xtyp(::Type{Complex128}) = COMPLEX
xtyp(::Type{ComplexF32}) = COMPLEX
xtyp(::Type{ComplexF64}) = COMPLEX

Copy link
Member Author

Choose a reason for hiding this comment

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

update alignment

base/complex.jl Outdated
const Complex32 = Complex{Float16}
const ComplexF64 = Complex{Float64}
const ComplexF32 = Complex{Float32}
const ComplexF16 = Complex{Float16}
Copy link
Member Author

Choose a reason for hiding this comment

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

update alignment

((:zgecon_,:Complex128,:Float64),
(:cgecon_,:Complex64, :Float32))
((:zgecon_,:ComplexF64,:Float64),
(:cgecon_,:ComplexF32, :Float32))
Copy link
Member Author

Choose a reason for hiding this comment

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

update alignment

((:Complex128, :Float64, :znaupd_, :zneupd_),
(:Complex64, :Float32, :cnaupd_, :cneupd_))
((:ComplexF64, :Float64, :znaupd_, :zneupd_),
(:ComplexF32, :Float32, :cnaupd_, :cneupd_))
Copy link
Member Author

Choose a reason for hiding this comment

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

update alignment

@antoine-levitt
Copy link
Contributor

Not sure if triage closes the discussion definitely, but FWIW I don't like the added F. It's ugly, and its only advantage is to clarify that it's a floating point and not an integer type. Is that really a serious confusion to anyone? (there's https://en.wikipedia.org/wiki/Gaussian_integer, but that seems pretty esoteric...). Or is the intention to use ComplexF to ease deprecation, then switch to Complex again?

@rfourquet
Copy link
Member Author

Or is the intention to use ComplexF to ease deprecation, then switch to Complex again?

I don't think there is such a plan.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Dec 8, 2017

There was fairly broad support that using the bitsize of the entire type in ComplexN was confusing to many people, and those who didn't mind it were only ambivalent. There's an unfortunate lack of consensus of what names like "complex64" means in different languages – in NumPy and Go it means Complex{Float32} whereas in Rust it means Complex{Float64}. The new names are unambiguous: the F64 in ComplexF64 can only refer to Float64 and therefore must mean Complex{Float64}. Note that ComplexF64 and Complex128 are the same length, and while ComplexF32 is one character longer than Complex64; the lack of ambiguity seems worth it.

@rfourquet
Copy link
Member Author

I will rebase and merge by tomorrow if no-one objects.

@garrison
Copy link
Sponsor Member

garrison commented Dec 8, 2017

in NumPy and Go [Complex64] means Complex{Float32}

Also in Mathematica. As far as I can tell, Rust is the odd one out.

@stevengj
Copy link
Member

stevengj commented Dec 8, 2017

In Fortran complex*2n is likewise used for complex numbers made of real*n, although in that case n is in bytes.

That being said, I guess I’m fine with the proposed new names too.

@StefanKarpinski
Copy link
Sponsor Member

Let's let this simmer for a bit longer.

@rfourquet
Copy link
Member Author

rfourquet commented Dec 9, 2017

If subjectivity could be an argument, I felt a nice stress relief when renaming Complex128 to ComplexF64 😜
Joke aside, I wonder how generic the languages having chosen Complex128 are... And even if Complex{Int} seems to be niche, it will be used. What I really like about ComplexF64 is that it sets a precedent in naming conventions: the package implementing quaternions will have QuaternionF64 be unambiguous, but also PlanePointF64 or SpacePointI32 will be easily interpreted etc.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Dec 9, 2017

Well, I'm convinced 😀. It is clearly a much more generalizable convention and we may as well set a good example in Base for the rest of the ecosystem to follow.

@garrison
Copy link
Sponsor Member

CFloat64 is an alternative that is more generalizable in some ways, and there are at least a few languages using this name. But I think it is a complete non-starter for julia, due to potential confusion with the existing Cint, Cdouble, etc. types.

@StefanKarpinski
Copy link
Sponsor Member

CFloat64 would definitely be confusing with our Cint etc. convention. I also think that the most salient part of the Complex{Float64} type is that it's complex, not that it's based on Float64.

@garrison
Copy link
Sponsor Member

garrison commented Dec 11, 2017

even if Complex{Int} seems to be niche, it will be used. [...] also PlanePointF64 or SpacePointI32

This is outside the scope of the current pull request, but because the proposed naming convention suggests other aliases as well, how do people feel about the following?

const ComplexI8 = Complex{Int8}
const ComplexI16 = Complex{Int16}
const ComplexI32 = Complex{Int32}
const ComplexI64 = Complex{Int64}
const ComplexI128 = Complex{Int128}
const ComplexI = Complex{Int}

@garrison
Copy link
Sponsor Member

As for myself, I've removed my downvote of this PR, as my objection was to the original plan of removing the aliases entirely. (At this point, my remaining bikeshedding is really about making sure we have the best possible aliases.)

@rfourquet
Copy link
Member Author

Freshly rebased, looking for someone to do the honors!

@musm
Copy link
Contributor

musm commented Dec 13, 2017

Personally, I don't feel like the code churn is really worth the renaming from Complex{N} to ComplexF{N}, even so considering that the F postfix in ComplexF isn't exactly standard and could be confusing to beginners in numerical computing.

@KristofferC
Copy link
Sponsor Member

Personally, I don't feel like the code churn is really worth the renaming from Complex{N} to ComplexF{N}

That's not what this PR does though?

@StefanKarpinski StefanKarpinski merged commit 24fce75 into master Dec 13, 2017
@StefanKarpinski StefanKarpinski deleted the rf/complex128 branch December 13, 2017 15:45
@Keno
Copy link
Member

Keno commented Dec 13, 2017

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:complex Complex numbers kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.