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.

Sign up for GitHub

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

Jump to bottom

deprecate array-reducing isinteger #19925

Merged
merged 1 commit into from
Jan 15, 2017
Merged

deprecate array-reducing isinteger #19925

merged 1 commit into from
Jan 15, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 7, 2017 •
edited
Loading

This pull request deprecates array-reducing isinteger(A::AbstractArray) in favor of all(isinteger, A) (edit: likewise isreal). Discussion: #19674 (comment), #19598 (comment), and #19598 (comment). (Edit: Also fixes isimag for real numbers that are zero.) Best!

@Sacha0 Sacha0 added the domain:broadcast Applying a function over a collection label Jan 7, 2017
@Sacha0 Sacha0 added this to the 0.6.0 milestone Jan 8, 2017
@kshyatt kshyatt added the kind:deprecation This change introduces or involves a deprecation label Jan 8, 2017
JeffBezanson
JeffBezanson reviewed Jan 8, 2017
View reviewed changes
base/abstractarraymath.jl
@@ -2,8 +2,6 @@

## Basic functions ##

isinteger(x::AbstractArray) = all(isinteger,x)
isinteger{T<:Integer,n}(x::AbstractArray{T,n}) = true
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

With this change, we will lose this optimization. It seems to be blocked by the bounds check in all; adding @inbounds to all allows the loop to be optimized away.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We might want to add

all{T<:Integer}(::typeof(isinteger), ::AbstractArray{T}) = true
all{T<:Real}(::typeof(isreal), ::AbstractArray{T}) = true

Copy link
Member Author

Choose a reason for hiding this comment

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

As opposed or in addition to decorating all with @inbounds as you suggest above? Thanks!

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think adding @inbounds to all is not safe unless we add a specialization for Array (where we know iteration never goes out of bounds).

Copy link
Member Author

Choose a reason for hiding this comment

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

Specializations added. Thanks!

@JeffBezanson
Copy link
Sponsor Member

Also isreal?

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 8, 2017

Also isreal?

Re. isreal, see #19598 (comment). Best!

@JeffBezanson
Copy link
Sponsor Member

There seems to be a good argument to keep iszero for arrays (though it's not exported), but it doesn't seem to apply to isreal.

isimag is strange. It's not vectorized, but also not used anywhere in Base. I also believe it has a bug:

julia> isimag(0)
true

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 8, 2017

Added one commit deprecating vectorized isreal, and another fixing isimag for real numbers that are zero. Thanks!

@Sacha0 Sacha0 added the kind:bugfix This change fixes an existing bug label Jan 9, 2017
@Sacha0 Sacha0 changed the title deprecate array-reducing isinteger deprecate array-reducing isinteger and isreal, fix isimag Jan 9, 2017
@JeffBezanson
Copy link
Sponsor Member

Should isimag(0+0im) be true?

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 9, 2017

Should isimag(0+0im) be true?

Per the docstring (isimag(z) -> Bool ... Test whether z is purely imaginary, i.e. has a real part equal to 0.), yes. Thoughts on whether both the docstring and behavior should change? (No thoughts on this end.) Best!

@TotalVerb
Copy link
Contributor

TotalVerb commented Jan 9, 2017 •
edited
Loading

If isreal(0+0im) is true, and isimag(0+0im) is true, why shouldn't isimag(0) be true? 0 is a purely imaginary number.

@TotalVerb
Copy link
Contributor

It seems misleading to me to claim isimag(0) to be false, at least, since this is a number with real part zero, hence purely imaginary. The only sensible options are to return true (which I prefer) or to throw an error for all subtypes of Real.

@JeffBezanson
Copy link
Sponsor Member

Saying "0 is an imaginary number" seems mildly crazy to me, but then again I'm not sure what this function is even for. There are no uses in Base. Are there example use cases that clarify which behavior would be desired?

@JeffBezanson
Copy link
Sponsor Member

Also @Sacha0 you'll probably want to move the isimag change to a separate PR since it's not related to vectorization, and we haven't figured out what the right behavior is yet.

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 9, 2017

Also @Sacha0 you'll probably want to move the isimag change to a separate PR since it's not related to vectorization, and we haven't figured out what the right behavior is yet.

Sounds wise. Dropping the last commit and opening an issue re. isimag. Thanks!

@Sacha0 Sacha0 mentioned this pull request Jan 9, 2017
Closed
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 9, 2017

Last commit dropped and issue #19947 opened. Are the vectorized isinteger and isreal deprecations in shape? Thanks!

@Sacha0 Sacha0 changed the title deprecate array-reducing isinteger and isreal, fix isimag deprecate array-reducing isinteger and isreal Jan 10, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 10, 2017

CI failures were #19953 (logs), restarted. Best!

@Sacha0 Sacha0 removed the kind:bugfix This change fixes an existing bug label Jan 10, 2017
@stevengj
Copy link
Member

Upon reflection, I don't think we should deprecate isreal(array), for the same reason we don't deprecate real(array) or conj(array). Complex conjugation and real and imaginary parts are part of the standard algebra of vectors/arrays of complex numbers. And if we define real(array), we should have isreal(array) (in the same way that if we have zero(array) we should have iszero(array)).

isinteger(array) has no such justification, as far as I can tell.

@Sacha0 Sacha0 mentioned this pull request Jan 12, 2017
Closed
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 12, 2017

I separated the array-reducing isreal deprecation into #19996 for further discussion; this pull request deprecates only array-reducing isinteger again. Best!

@Sacha0 Sacha0 changed the title deprecate array-reducing isinteger and isreal deprecate array-reducing isinteger Jan 12, 2017
tkelman
tkelman reviewed Jan 14, 2017
View reviewed changes
base/abstractarraymath.jl
isreal(x::AbstractArray) = all(isreal,x)
iszero(x::AbstractArray) = all(iszero,x)
isreal{T<:Real,n}(x::AbstractArray{T,n}) = true
all{T<:Integer}(::typeof(isinteger), ::AbstractArray{T}) = true
ctranspose(a::AbstractArray) = error("ctranspose not implemented for $(typeof(a)). Consider adding parentheses, e.g. A*(B*C') instead of A*B*C' to avoid explicit calculation of the transposed matrix.")
transpose(a::AbstractArray) = error("transpose not implemented for $(typeof(a)). Consider adding parentheses, e.g. A*(B*C.') instead of A*B*C' to avoid explicit calculation of the transposed matrix.")
Copy link
Contributor

Choose a reason for hiding this comment

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

bump for rebase. I think these two fallbacks are gone on master? or just moved, not positive

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased. Thanks!

@tkelman tkelman merged commit ce5a62a into JuliaLang:master Jan 15, 2017
@Sacha0 Sacha0 deleted the depisinteger branch January 15, 2017 05:02
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 15, 2017

Thanks all!

@StefanKarpinski StefanKarpinski mentioned this pull request Jan 26, 2017
Closed
22 tasks
@Sacha0 Sacha0 mentioned this pull request Feb 3, 2017
Merged
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017
@Sacha0
Add NEWS.md entry for deprecation of array-reducing isinteger (JuliaL…
8459648 
…ang#19925).
@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
@Sacha0 @tkelman
Add NEWS.md entry for deprecation of array-reducing isinteger (#19925)…
da60928 
…. (#21856)
@tkelman tkelman removed the needs news A NEWS entry is required for this change label May 14, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
@Sacha0 @tkelman
Add NEWS.md entry for deprecation of array-reducing isinteger (#19925)…
f2fcc03 
…. (#21856)

(cherry picked from commit da60928)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffBezanson JeffBezanson JeffBezanson left review comments

@tkelman tkelman tkelman left review comments

Assignees
No one assigned
Labels
domain:broadcast Applying a function over a collection kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Sacha0 @JeffBezanson @TotalVerb @stevengj @tkelman @kshyatt