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

Deprecate vectorized two-argument complex in favor of compact broadcast syntax #19712

Merged
merged 1 commit into from
Jan 1, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Dec 25, 2016

This pull request deprecates most remaining two-argument complex methods (less a few defined in base/sparse/sparsevectors.jl that depend on #19690) in favor of compact broadcast syntax. Best!

(Two-argument vectorized complex methods always return a fresh array. So in contrast to one-argument vectorized complex methods (which suffer from the same controversy as float, namely what to do about ad hoc aliasing semantics as in #18495), these deprecations should be uncontroversial.)

@@ -512,8 +512,6 @@ function copy!(S::SharedArray, R::SharedArray)
return S
end

complex(S1::SharedArray,S2::SharedArray) = convert(SharedArray, complex(S1.s, S2.s))
Copy link
Contributor

Choose a reason for hiding this comment

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

complex.(S1, S2) would be at the mercy of similars return type here, right? so would not return a SharedArray any more unless we were to go back on similar returning SharedArray ?

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to define a separate broadcast method, like

broadcast{T<:SharedArray}(::typeof(complex), S1::T, S2::T) = convert(T, complex.(S1.s, S2.s))

?

Copy link
Member Author

Choose a reason for hiding this comment

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

complex.(S1, S2) would be at the mercy of similars return type here, right? so would not return a SharedArray any more unless we were to go back on similar returning SharedArray ?

IIUC, yes, complex.(A, B) for SharedArrays A and B would yield an appropriate <:Array rather than <:SharedArray. Best to keep this method as a broadcast specialization as @ararslan suggests, or instead require the more explicit convert(SharedArray, complex.(A, B)) going forward? (Fused cases will require the latter in any case unless we build a generic broadcast for SharedArrays.) Thanks!

Copy link
Member

@stevengj stevengj Dec 27, 2016

Choose a reason for hiding this comment

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

I would not recommend defining broadcast(::typeof(complex), ...), because it will not be called in the presence of fusion etcetera.

If you want a SharedArray output, one option is to use in-place assignment (mysharedarray .= complex.(a,b)). Another possibility would to define Base.Broadcast.containertype promotion methods to always produce a SharedArray from broadcast on SharedArray inputs, if that is the desired behavior.

@Sacha0 Sacha0 added the broadcast Applying a function over a collection label Dec 25, 2016
@Sacha0 Sacha0 added this to the 0.6.0 milestone Dec 25, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Dec 31, 2016

Before I spend more time on this pull request: @stevengj, are you in favor of this change (as opposed to deprecating single-argument complex)? Thanks!

@stevengj
Copy link
Member

Yes, for the two-argument complex(re,im), it seems unambiguous that we should prefer a dot call to vectorize it.

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 1, 2017

Revised extensively. I expect a round or three of CI failures. (Edit: Shockingly CI passed immediately.) Best!

@tkelman tkelman merged commit 205c12b into JuliaLang:master Jan 1, 2017
@Sacha0 Sacha0 deleted the devectwoargcomplex branch January 7, 2017 04:40
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants