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

tbd: deprecate array-reducing isreal #19996

Closed
wants to merge 1 commit into from
Closed

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 12, 2017

Deprecation of array-reducing isreal(A::AbstractArray) in favor of all(isinteger, A) sparked debate in #19925, so I've separated that deprecation (into this pull request) from #19925's uncontroversial isinteger deprecation. isreal(A::AbstractArray): keep or nix? Best!

@Sacha0 Sacha0 added the broadcast Applying a function over a collection label Jan 12, 2017
@ararslan
Copy link
Member

Personally I'd be in favor of doing away with the array-reducing isreal method since, at least IMHO, the intent of all(isreal, A) is clearer. But if we do this, we should similarly deprecate the array-reducing methods of other functions such as iszero for consistency.

@@ -509,7 +509,7 @@ function logm(A::StridedMatrix)
end
end

if isreal(A) && ~np_real_eigs
if all(isreal, A) && ~np_real_eigs
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated, but shouldn't this be !np_real_eigs instead of ~np_real_eigs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would have thought so, but apparently ~(x::Bool) is a synonym for !x (~ being bitwise not)?

Copy link
Contributor

@tkelman tkelman Jan 12, 2017

Choose a reason for hiding this comment

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

they happen to be equivalent for Bool, but I suspect this was written more out of matlab-habit than intentionally

Copy link
Member Author

@Sacha0 Sacha0 Jan 12, 2017

Choose a reason for hiding this comment

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

Agreed. Will touch that up if this pull request goes forward. Otherwise will open a touchup PR. Thanks!

@kshyatt kshyatt added the deprecation This change introduces or involves a deprecation label Jan 12, 2017
@stevengj
Copy link
Member

stevengj commented Jan 12, 2017

My basic argument here is that isreal is a meaningful algebraic question for an array itself — one views the array as a complex algebraic object in its own right, not just as a container for things that have their own algebras (see e.g. complex structure/complexification of vector spaces). This is why we should define real, conj, etcetera for arrays, and for the same reason it is why we should define isreal.

It's analogous to the fact that we define array * scalar, even though this is technically redundant with array .* scalar from a programming standpoint: it's because we want to view arrays mathematically as forming a vector space. Similarly, I think we also want to be able to view complex arrays as a complexified vector space.

@andyferris
Copy link
Member

Is all(isreal, x) as fast as e.g. eltype(x) <: Real when that is satisfied?

@stevengj has a point. In common mathematical parlance, you can have a real matrix and you can have a complex matrix. There isn't, in my view, a great deal of semantic mathematical difference between Complex{Matrix{Float64}} and Matrix{Complex{Float64}} - it's just a matter of practicality.

@Sacha0 Sacha0 added the needs decision A decision on this change is needed label Jan 15, 2017
@Sacha0 Sacha0 closed this Feb 13, 2017
@Sacha0 Sacha0 deleted the depisreal branch February 21, 2017 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection deprecation This change introduces or involves a deprecation needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants