Skip to content

Conversation

@simonbyrne
Copy link
Member

Allows for other complex number types, e.g. PolarComplex representations or Duals for Wirtinger derivatives.

Allows for other complex number types, e.g. PolarComplex representations or Duals for Wirtinger derivatives.
base/complex.jl Outdated
Abstract supertype for complex numbers.
"""
abstract type AbstractComplex <: Number
end
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put it on the line above? Having it on a separate line seems as if we might add fields which we can't.

@nalimilan
Copy link
Member

Would it make sense to have Real <: AbstractComplex? I think we touched this in a discussion the other day but I don't remember the details.

@simonbyrne
Copy link
Member Author

I think we decided against it, as we decided it wasn't really a subset thing (but again can't remember the details).

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Mar 30, 2018

Real <: AbstractComplex is incorrect because the reals are isomorphic to a subset of the reals, not identical to; and in computer implementations we need to be fussy about that sort of thing. Perhaps a way to understand this more clearly is instead of reading Real <: AbstractComplex as saying that "real numbers are a subset of complex numbers", read it as "implementations of real numbers are a subset of the implementations of complex numbers"—which is clearly not the case.

@mbauman
Copy link
Member

mbauman commented Mar 30, 2018

I'd like to see a bit more development here before we merge this — particularly if it's going to land this late in a release cycle. This exports a new abstract type that folks are going to want to use, but it provides no guidance on what it means beyond the name.

What methods should I implement? What methods are provided? How does it promote? How does it convert? What does this provide beyond just subtyping <: Number? Why would you choose to dispatch on AbstractComplex?

Every time I’ve gone to actually implement an abstract supertype things are more challenging than they seem. It often requires re-jiggering the base dispatch trees. I'm imagining that iterating on this in a non-breaking manner may prove to be difficult.

@mbauman mbauman added needs tests Unit tests are required for this change needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Mar 30, 2018
@simonbyrne
Copy link
Member Author

Those are good points.

@StefanKarpinski StefanKarpinski added this to the 1.x milestone Mar 30, 2018
@garrison
Copy link
Member

Also related to discussion in #26550.

@TotalVerb
Copy link
Contributor

@StefanKarpinski While I don't disagree in principle, are there any functions that would work on complex numbers that don't make sense or behave differently on real numbers? Real numbers certainly implement the interface needed for AbstractComplex (real, imag, angle, abs, etc.). I think Real <: AbstractComplex would simplify the definition of methods that operate on complex numbers... since they would generally work for real numbers too.

In other words, while strictly speaking implementations of the reals are not a subset of implementations of the complex numbers, all implementations of real numbers should be able to behave like complex numbers. So why not Real <: AbstractComplex?

@jw3126
Copy link
Contributor

jw3126 commented Apr 3, 2018

@TotalVerb sqrt is a function that behaves differently on Real and Compex numbers.

julia> sqrt(-1)
ERROR: DomainError:
sqrt will only return a complex result if called with a complex argument. Try sqrt(complex(x)).
Stacktrace:
 [1] sqrt(::Int64) at ./math.jl:434

julia> sqrt(Complex(-1))
0.0 + 1.0im

@TotalVerb
Copy link
Contributor

@jw3126 That's a good point. Yet we have

julia> (Base.:^)(2, -1)
ERROR: DomainError with -1:
Cannot raise an integer x to a negative power -1.
Make x a float by adding a zero decimal (e.g., 2.0^-1 instead of 2^-1), or write 1/x^1, float(x)^-1, or (x//1)^-1
Stacktrace:
 [1] throw_domerr_powbysq(::Int64, ::Int64) at ./intfuncs.jl:176
 [2] power_by_squaring(::Int64, ::Int64) at ./intfuncs.jl:196
 [3] ^(::Int64, ::Int64) at ./intfuncs.jl:220
 [4] top-level scope

and still Integer <: Real.

@jw3126
Copy link
Contributor

jw3126 commented Apr 4, 2018

@TotalVerb Fair enough. Here is another issue: We can canonically embed the reals into many things, why not Real <: DualNumber instead?
OTOH I think you are right that we should use whatever sub-typing leads to the cleanest code and least surprises. Maybe that's indeed Real <: AbstractComplex, really not sure.

@nalimilan
Copy link
Member

The difference is that DualNumber is not defined in Base.

@oscardssmith
Copy link
Member

Shouldn't it be that AbstractComplex <: Real? I know this seems counterintuitive, but any Complex{T} behaves as a subtype of T, even though T is a subset of Complex{T}.

@TotalVerb
Copy link
Contributor

@oscardssmith This is not correct; for example < is defined for ::Real but not for ::Complex. Any implementations of functions that involve comparison (for example, a bisection algorithm) that dispatch on ::Real would not work for ::Complex.

@StefanKarpinski
Copy link
Member

Well, I think the point is proven that we should not do this hastily or without further consideration.

@TotalVerb
Copy link
Contributor

TotalVerb commented Apr 5, 2018

@StefanKarpinski Did you mean to close the PR? It just adds AbstractComplex, and we can always add Real <: AbstractComplex at a later time, without needing it to be part of the PR.

@StefanKarpinski
Copy link
Member

Yes, it still needs tests, news, etc. and as I said, I don't think we should do this now. We can always reopen it later when we decide that we want to do this.

@kimikage kimikage mentioned this pull request May 1, 2020
@DilumAluthge DilumAluthge deleted the sb/abstractcomplex branch March 25, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants