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

Remove broadcast_elwise_op and deprecate promote_eltype_op #19814

Merged
merged 2 commits into from
Jan 4, 2017
Merged

Remove broadcast_elwise_op and deprecate promote_eltype_op #19814

merged 2 commits into from
Jan 4, 2017

Conversation

martinholters
Copy link
Member

AFAICT, broadcast_elwise_op was only used in the deprecation of _broadcast_zpreserving.

I've found the following uses of promote_eltype_op:

  • ImageFiltering - only adds methods, should be no problem.
  • DataArrays - actually uses Compat.promote_eltype_op.
  • Compat - Defines its own version or re-exports the one from Base if it exists.

Probably the cleanest thing would be to update DataArrays not to rely on promote_eltype_op and also deprecate it in Compat. Or Compat could just use the definition from the deprecation directly.

tkelman
tkelman reviewed Jan 2, 2017
View reviewed changes
base/deprecated.jl
@@ -1470,4 +1470,7 @@ end
@deprecate (|)(A::AbstractArray, b::Number) A .| b
@deprecate (|)(A::AbstractArray, B::AbstractArray) A .| B

# Calling promote_op is likely a bad idea, so deprecate to its convenience wrapper promote_eltype_op
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment backwards?

Copy link
Member Author

Choose a reason for hiding this comment

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

Errr... should read "so deprecate its convenience wrapper". Does that make sense?

@martinholters
Copy link
Member Author

Oh, the travis i686 job failed due to

julia: /home/travis/build/JuliaLang/julia/deps/srccache/llvm-3.9.1/lib/MC/MCAsmStreamer.cpp:384: virtual void {anonymous}::MCAsmStreamer::EmitLabel(llvm::MCSymbol*): Assertion `Symbol->isUndefined() && "Cannot define a symbol twice!"' failed.
signal (6): Aborted
while loading /tmp/julia/share/julia/test/reflection.jl, in expression starting on line 43
__kernel_vsyscall at  (unknown line)
gsignal at /lib/i386-linux-gnu/libc.so.6 (unknown line)
abort at /lib/i386-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0xf725ed14)
__assert_fail at /lib/i386-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0xf5b57a7f)
unknown function (ip: 0xf72036bb)
unknown function (ip: 0xec83016a)
Allocations: 28517363 (Pool: 28515574; Big: 1789); GC: 21

Full log is here: https://gist.github.com/martinholters/362f5507ee7ded7b59f043c74cc52fbe

@martinholters
Copy link
Member Author

Ah, that assertion failure is also what #19803 is about if I'm not mistaken, so unrelated to this PR?

@kshyatt kshyatt added the domain:types and dispatch Types, subtyping and method dispatch label Jan 2, 2017
@pabloferz
Copy link
Contributor

LGTM

@pabloferz pabloferz mentioned this pull request Jan 3, 2017
Closed
22 tasks
@martinholters
Copy link
Member Author

Good to go or are there any objections against this?

@tkelman tkelman merged commit 2e08102 into master Jan 4, 2017
@tkelman tkelman deleted the mh/dep_p_e_o branch January 4, 2017 08:12
@TotalVerb
Copy link
Contributor

Note that the existence of this function is not because of convenience, but because the suggested replacement is type-unstable. This will hammer performance of packages using this.

TotalVerb
TotalVerb reviewed Jan 8, 2017
View reviewed changes
base/deprecated.jl
@@ -1239,7 +1239,7 @@ for (Bsig, A1sig, A2sig, gbb, funcname) in
end # let broadcast_cache
end
_broadcast_zpreserving!(args...) = broadcast!(args...)
_broadcast_zpreserving(args...) = Base.Broadcast.broadcast_elwise_op(args...)
_broadcast_zpreserving(f, As...) = broadcast!(f, similar(Array{promote_op(f, map(eltype, As)...)}, Base.Broadcast.broadcast_indices(As...)), As...)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems bad to have a type-unstable implementation, even if deprecated.

@TotalVerb TotalVerb mentioned this pull request Jan 8, 2017
Merged
@Sacha0
Copy link
Member

Sacha0 commented Jan 8, 2017

Note that the existence of this function is not because of convenience, but because the suggested replacement is type-unstable. This will hammer performance of packages using this.

Along similar lines, promote_op(f, map(eltype, As)...) is not equivalent to promote_eltype_op(f, As...) where As contains more than two arguments. A gentler deprecation might be worthwhile. Best!

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 20, 2017
@Sacha0
Add NEWS.md entry for promote_eltype_op deprecation (JuliaLang#19814, J…
d265d00 
…uliaLang#19937).
@Sacha0 Sacha0 added kind:deprecation This change introduces or involves a deprecation needs news A NEWS entry is required for this change labels May 20, 2017
tkelman pushed a commit that referenced this pull request May 24, 2017
@Sacha0 @tkelman
Add NEWS.md entry for promote_eltype_op deprecation (#19814, #19937). (…
152d255 
…#21992)
tkelman pushed a commit that referenced this pull request Jun 3, 2017
@Sacha0 @tkelman
Add NEWS.md entry for promote_eltype_op deprecation (#19814, #19937). (…
9129a3c 
…#21992)

(cherry picked from commit 152d255)
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

@TotalVerb TotalVerb TotalVerb left review comments

Assignees
No one assigned
Labels
domain:types and dispatch Types, subtyping and method dispatch kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@martinholters @pabloferz @TotalVerb @Sacha0 @tkelman @kshyatt @KristofferC