Skip to content

Conversation

@Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jul 6, 2016

This PR deprecates all functions vectorized via macros vectorize_1arg and vectorize_2arg in favor of compact broadcast syntax (ref. #16285). Many non-macro-vectorized functions remain for a separate PR. Should this PR deprecate the vectorize_1arg and vectorize_2arg macros themselves alongside their usage in base? Best!

Ref. #17265 (devectorization of functions over SparseMatrixCSCs).

@nalimilan
Copy link
Member

+100 if we are sure this won't affect performance. I guess this depends on promote_op methods being correctly defined for all these operations/types...

@stevengj
Copy link
Member

stevengj commented Jul 7, 2016

I feel like this is best delayed for 0.6 — it's going to lead to a huge number of deprecation warnings in packages.

@JeffBezanson
Copy link
Member

I guess this depends on promote_op methods being correctly defined for all these operations/types

Indeed, the promote_op approach does not scale at all.

:airyprime, :airyai, :airyaiprime, :airybi, :airybiprime, :airy, :airyx, :besselj0, :besselj1, :bessely0, :bessely1, # base/special/bessel.jl
:cbrt, :sinh, :cosh, :tanh, :atan, :asinh, :exp, :erf, :erfc, :exp2, :expm1, :exp10, :sin, :cos, :tan, :asin, :acos, :acosh, :atanh, #=:log,=# :log2, :log10, :lgamma, #=:log1p,=# :sqrt, # base/math.jl
:abs, :abs2, :angle, :isnan, :isinf, :isfinite, # base/floatfuncs.jl
:acos_fast, :acosh_fast, :angle_fast, :asin_fast, :asinh_fast, :atan_fast, :atanh_fast, :cbrt_fast, :cis_fast, :cos_fast, :cosh_fast, :exp10_fast, :exp2_fast, :exp_fast, :expm1_fast, :lgamma_fast, :log10_fast, :log1p_fast, :log2_fast, :log_fast, :sin_fast, :sinh_fast, :sqrt_fast, :tan_fast, :tanh_fast, # base/fastmath.jl
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that these are annotated with where they came from, but should spread over multiple lines when it gets this long

@Sacha0
Copy link
Member Author

Sacha0 commented Jul 8, 2016

I feel like this is best delayed for 0.6 — it's going to lead to a huge number of deprecation warnings in packages.

Agreed. This change should wait for 0.6 for several reasons. Best!

@stevengj stevengj added this to the 0.6.0 milestone Jul 21, 2016
@stevengj stevengj added the broadcast Applying a function over a collection label Aug 2, 2016
@stevengj
Copy link
Member

stevengj commented Aug 2, 2016

Note that this will close #4363.

@stevengj stevengj mentioned this pull request Aug 2, 2016
17 tasks
@Sacha0 Sacha0 force-pushed the vectobroad branch 3 times, most recently from 545773a to 166c97a Compare August 6, 2016 17:21
@Sacha0
Copy link
Member Author

Sacha0 commented Aug 6, 2016

Rebased for 0.6 and passing CI. Nanosoldier? Best!

@musm
Copy link
Contributor

musm commented Aug 6, 2016

How does this affect performance? I think a nanosoldier run could be worthwhile

@KristofferC
Copy link
Member

There are very few benchmarks for sparse arrays in BaseBenchmarks.jl.

@Sacha0 Sacha0 changed the title WIP: Deprecate functions vectorized via @vectorize_(1|2)arg in favor of compact broadcast syntax Deprecate functions vectorized via @vectorize_(1|2)arg in favor of compact broadcast syntax Aug 7, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Aug 9, 2016

@KristofferC, most functionality this pull request touches is not immediately related to sparse arrays. Conjecturing the contrary was reasonable though, given my predilections :).

@KristofferC
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@Sacha0
Copy link
Member Author

Sacha0 commented Aug 10, 2016

Rebased resolving a deprecation conflict. Prior to rebase, this PR passed both AppVeyor and Travis. Nanosoldier appears happy as well. (As @pabloferz pointed out in #17929 (comment), the lucompletepiv regressions seems unrelated.) Is this PR in good shape? Thanks!

@stevengj
Copy link
Member

Test failure due to use of abs(x) in test/inference.jl:280.

@stevengj
Copy link
Member

Might require some manual updates as well?

@kshyatt kshyatt added the needs docs Documentation for this change is required label Aug 10, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Sep 4, 2016

Rebased. Thanks for the ping!

@tkelman tkelman merged commit 4533900 into JuliaLang:master Sep 4, 2016
vtjnash referenced this pull request Sep 5, 2016
@Sacha0 Sacha0 deleted the vectobroad branch September 10, 2016 18:31
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Sep 12, 2016
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Sep 12, 2016
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Nov 10, 2016
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Nov 21, 2016
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Mar 16, 2017
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 needs docs Documentation for this change is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.