-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
1d090af
to
d057b8e
Compare
base/linalg/blas.jl
Outdated
@@ -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} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Is it the plan to deprecate the current IIUC |
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. |
base/sparse/umfpack.jl
Outdated
@@ -15,6 +15,8 @@ struct MatrixIllConditionedException <: Exception | |||
msg::AbstractString | |||
end | |||
|
|||
const Complex128 = Complex{Float64} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
base/linalg/lapack.jl
Outdated
@@ -14,6 +14,9 @@ import ..LinAlg: BlasFloat, Char, BlasInt, LAPACKException, | |||
|
|||
using Base: iszero | |||
|
|||
const Complex64 = Complex{Float32} |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
This could indeed be more safe. One of the other reason I used these local aliase is that |
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. |
Consider using
or something of that sort. |
Even simpler: |
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. |
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! |
From what i could see, those types are used in linalg mainly for wrapping libraries, with loops and |
Other languages like maybe Go have these type names also, no? (Mostly, I would avoid the rename to |
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. |
No need to be that shy with the triage label, that's what it's for. We may as well discuss it. |
I'm fine with deprecating these types. We may want to introduce |
I've been thinking more about why I am opposed to this. My main objection is about 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 As for ambiguity, it is already possible to learn what
Best! [channeling @Sacha0 here... 😉] |
Regarding (Edit: Oh gosh, I forgot the...) Best! |
I agree that it could be better to still have "official" aliases. I'm not a big user of So I understand that the removal of |
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 Best! |
Perhaps Edit: Or even |
6285cbb
to
06f22ba
Compare
Triage decided to introduce new aliases, namely |
stdlib/SuiteSparse/src/cholmod_h.jl
Outdated
xtyp(::Type{Complex64}) = COMPLEX | ||
xtyp(::Type{Complex128}) = COMPLEX | ||
xtyp(::Type{ComplexF32}) = COMPLEX | ||
xtyp(::Type{ComplexF64}) = COMPLEX | ||
|
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update alignment
base/linalg/lapack.jl
Outdated
((:zgecon_,:Complex128,:Float64), | ||
(:cgecon_,:Complex64, :Float32)) | ||
((:zgecon_,:ComplexF64,:Float64), | ||
(:cgecon_,:ComplexF32, :Float32)) |
There was a problem hiding this comment.
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_)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update alignment
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? |
I don't think there is such a plan. |
There was fairly broad support that using the bitsize of the entire type in |
I will rebase and merge by tomorrow if no-one objects. |
Also in Mathematica. As far as I can tell, Rust is the odd one out. |
In Fortran That being said, I guess I’m fine with the proposed new names too. |
Let's let this simmer for a bit longer. |
If subjectivity could be an argument, I felt a nice stress relief when renaming |
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. |
|
|
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} |
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.) |
40081a0
to
4424e54
Compare
Freshly rebased, looking for someone to do the honors! |
Personally, I don't feel like the code churn is really worth the renaming from |
That's not what this PR does though? |
🎉 |
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: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 renamingComplex128
toComplex64
with the idea "the coordinates of aComplex64
isFloat64
", 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);Complex128
type actually precededComplex{Float64}
— it was originally a bits type because Julia didn’t have immutable types. So, at this point, deprecatingComplex128
makes a lot of sense (@stevengj).