-
-
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
Deprecate vectorized float methods in favor of compact broadcast syntax #18495
Conversation
end | ||
convert(AbstractArray{typeof(float(zero(T)))}, A) | ||
# float, broadcast over arrays | ||
broadcast{T<:AbstractFloat}(::typeof(float), A::AbstractArray{T}) = A |
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.
Probably we should also have broadcast{T<:AbstractFloat}(::typeof(float), A::AbstractArray{Complex{T}}) = A
in complex.jl
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.
Added. Thanks!
CI appears happy now, so I've nixed the WIP tag / this should be ready for review. Best! |
I'm not sure how I feel about |
Likewise, and not that I am aware of (though I have not searched). The new broadcast methods are direct translations of (and hence drop-in replacements for) the former |
In all the cases where we use |
Somewhat perversely, however, the loop fusion means that |
Agreed. On the other hand, having |
I'm not totally sure that these should be |
We could put it in the |
The more I think it through, the more I err on the side of |
I am strongly against I am more undecided about whether |
Also, unless this makes it into 0.5.0, it is technically breaking (unless we decide that code that relied on |
Good point! My thinking was specious. The fusion mechanism's behavior is (and probably should remain) unspecified, so the trick I proposed is precarious.
These statements further underscore your point (though perhaps inadvertently): Note that
I would advocate against
My primary concern was performance, particularly what
and leaves the door open for more exotic fusion-like behaviors, for example those discussed in #16285 (potentially involving intermediary lazy maps / broadcasts / reductions). But after some more thinking, perhaps that concern was specious too: (edit: <skip for better flow>)Consider a vectorized Regarding performance, suppose a vectorized Now instead suppose the vectorized Thus (2) seems like the only case of (near-term, practical) concern. I audited the usages changed in this PR and could not find an instance where So how does this sound: For now, replace no-op vectorized methods such as |
(Actually, the fusion mechanism's behavior is specified in the manual. This is necessary because it affects the semantics of dot calls when there are side effects, so it is guaranteed that fusing always occurs.) |
I don't think it is unreasonable to simply document (in the |
As long as it's fully specified and documented, I don't think it's too bad to make What's the reason not to leave |
The argument for not leaving it as-is is that we're training users to use |
As @pabloferz notes in #18618 (comment),
The new syntax is fantastic, resulting in a drive to use it for everything. In this case we are trying to pack all existing vectorized methods into that new syntax. Most existing vectorized methods exhibit strict Perhaps
|
The observation that some of these functions really have float(x) = convert(@UnionAll(T<:Union{FloatingPoint,UnionAll(S<:FloatingPoint,Complex{S})},Array{T}), x)
big(x) = convert(Union{Array{BigInt},Array{BigFloat}}, x)
complex(x) = convert(@UnionAll(T<:Complex,Array{T}), x)
|
I wonder if the dot fusion should pass a tuple of the functions that were fused, which might help to propagate properties like zero-preservation in a type-stable way. But it feels too complicated. The thing is, in principle just checking |
Good point! Invariance. I had |
@stevengj, did you mean to post #18495 (comment) in #18590? Best! |
Whoops, right. |
Bump? |
Re. #18495 (comment), why not |
I would leave |
This argument for preserving It seems consensus favors deprecating vectorized |
Writing e.g. |
|
(Ref. #22236 (comment).) |
This PR deprecates all (?) remaining vectorized
float
methods in favor of compact broadcast syntax. Ref. #16285 and #17302. Best!To work around #18462, I made the
Broadcast
moduleimport
rather thanexport
broadcast
. I will correct this if / when a better solution for #18462 appears.