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 functions vectorized via `@vectorize_(1|2)arg` in favor of compact broadcast syntax #17302

Merged
merged 18 commits into from Sep 4, 2016

Conversation

Projects
None yet
@Sacha0
Member

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!

  • TODO: Vectorized functions involving dates/times. Done.
  • TODO: Prettify deprecation warnings. See #17289. Fixed in #17521.
  • TODO: Resolve issue with the last test in test/operators.jl. See #17304. Thanks @stevengj!
  • TODO: Resolve deprecation warnings issued by markdown's colwidths. @MikeInnes, how would you rewrite that line less vectorized max? My attempts broke markdown, so I imagine I don't understand the intention. Thanks!

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

@nalimilan

This comment has been minimized.

Show comment
Hide comment
@nalimilan

nalimilan Jul 7, 2016

Contributor

+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...

Contributor

nalimilan commented Jul 7, 2016

+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

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Jul 7, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Jul 7, 2016

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.

Member

JeffBezanson commented Jul 7, 2016

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.

Show outdated Hide outdated base/deprecated.jl
: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

This comment has been minimized.

@tkelman

tkelman Jul 7, 2016

Contributor

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

@tkelman

tkelman Jul 7, 2016

Contributor

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

Show outdated Hide outdated base/sysimg.jl
@@ -294,12 +294,12 @@ include("client.jl")
include("util.jl")
# dense linear algebra
include("broadcast.jl")
importall .Broadcast

This comment has been minimized.

@tkelman

tkelman Jul 7, 2016

Contributor

probably before the linear algebra comment since broadcast is a bit more general than that

@tkelman

tkelman Jul 7, 2016

Contributor

probably before the linear algebra comment since broadcast is a bit more general than that

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Jul 8, 2016

Member

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!

Member

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 label Aug 2, 2016

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Aug 2, 2016

Member

Note that this will close #4363.

Member

stevengj commented Aug 2, 2016

Note that this will close #4363.

@stevengj stevengj referenced this pull request Aug 2, 2016

Open

Implement more matrix functions #5840

5 of 17 tasks complete
@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 6, 2016

Member

Rebased for 0.6 and passing CI. Nanosoldier? Best!

Member

Sacha0 commented Aug 6, 2016

Rebased for 0.6 and passing CI. Nanosoldier? Best!

@musm

This comment has been minimized.

Show comment
Hide comment
@musm

musm Aug 6, 2016

Contributor

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

Contributor

musm commented Aug 6, 2016

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

@KristofferC

This comment has been minimized.

Show comment
Hide comment
@KristofferC

KristofferC Aug 6, 2016

Contributor

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

Contributor

KristofferC commented Aug 6, 2016

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

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

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 9, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@KristofferC

KristofferC Aug 9, 2016

Contributor

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

Contributor

KristofferC commented Aug 9, 2016

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

@nanosoldier

This comment has been minimized.

Show comment
Hide comment
@nanosoldier

nanosoldier Aug 9, 2016

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

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

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 10, 2016

Member

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!

Member

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

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Aug 10, 2016

Member

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

Member

stevengj commented Aug 10, 2016

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

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Aug 10, 2016

Member

Might require some manual updates as well?

Member

stevengj commented Aug 10, 2016

Might require some manual updates as well?

@kshyatt kshyatt added the needs docs label Aug 10, 2016

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 10, 2016

Member

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

Introduced by #17847. I'm not certain how to migrate that test to compact broadcast syntax (or otherwise modify it): That test checks that typeof(abs(Integer[])) == Array{Any,1} rather than Array{Union{},1}. And as @stevengj points out in #17847 (comment), broadcast does not use the logic the macro-vectorized abs call exercises. Hence migrating to compact broadcast syntax would not capture the test's intention (nor would it succeed, as typeof(abs.(Integer[])) == Arrray{Union{},1} presently). Thoughts? Thanks!

Member

Sacha0 commented Aug 10, 2016

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

Introduced by #17847. I'm not certain how to migrate that test to compact broadcast syntax (or otherwise modify it): That test checks that typeof(abs(Integer[])) == Array{Any,1} rather than Array{Union{},1}. And as @stevengj points out in #17847 (comment), broadcast does not use the logic the macro-vectorized abs call exercises. Hence migrating to compact broadcast syntax would not capture the test's intention (nor would it succeed, as typeof(abs.(Integer[])) == Arrray{Union{},1} presently). Thoughts? Thanks!

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Aug 10, 2016

Member

Can you file an issue to extend #17847 to map and broadcast?

Member

stevengj commented Aug 10, 2016

Can you file an issue to extend #17847 to map and broadcast?

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Aug 12, 2016

Contributor

I guess it can be done in a separate PR from this one, but I think deprecating the vectorize_1arg etc macros themselves would also make sense

Contributor

tkelman commented Aug 12, 2016

I guess it can be done in a separate PR from this one, but I think deprecating the vectorize_1arg etc macros themselves would also make sense

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Aug 12, 2016

Member

I agree, @tkelman. That is, you've already replaced all calls to @vectorize_1arg in Base with @dep_vectorize_1arg, and then add a new definition of @vectorize_1arg to deprecated.jl that prints a deprecation warning and then calls @dep_vectorize_1arg.

Member

stevengj commented Aug 12, 2016

I agree, @tkelman. That is, you've already replaced all calls to @vectorize_1arg in Base with @dep_vectorize_1arg, and then add a new definition of @vectorize_1arg to deprecated.jl that prints a deprecation warning and then calls @dep_vectorize_1arg.

Show outdated Hide outdated base/deprecated.jl
S = esc(S)
f = esc(f)
quote
@deprecate $f{T1<:$S,T2<:$S}(x::T1, y::AbstractArray{T2}) $f.(x,y)

This comment has been minimized.

@stevengj

stevengj Aug 12, 2016

Member

You can just use x::$S here, and similarly in the next line.

@stevengj

stevengj Aug 12, 2016

Member

You can just use x::$S here, and similarly in the next line.

This comment has been minimized.

@Sacha0

Sacha0 Aug 13, 2016

Member

Good catch. Simplified. Thanks!

@Sacha0

Sacha0 Aug 13, 2016

Member

Good catch. Simplified. Thanks!

Show outdated Hide outdated base/deprecated.jl
:daysofweekinmonth, :monthname, :monthabbr, :daysinmonth, # base/dates/query.jl
:isleapyear, :dayofyear, :daysinyear, :quarterofyear, :dayofquarter, # base/dates/query.jl
)
@eval @dep_vectorize_1arg Dates.TimeType $f

This comment has been minimized.

@stevengj

stevengj Aug 12, 2016

Member

Seems like this depends on #16966 — i.e. we need broadcast to recognize TimeType as a "scalar".

@stevengj

stevengj Aug 12, 2016

Member

Seems like this depends on #16966 — i.e. we need broadcast to recognize TimeType as a "scalar".

This comment has been minimized.

@stevengj

stevengj Aug 12, 2016

Member

Oh, no, I guess it will work as-is for operations on Array{TimeType} by themselves.

@stevengj

stevengj Aug 12, 2016

Member

Oh, no, I guess it will work as-is for operations on Array{TimeType} by themselves.

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 13, 2016

Member

Rebased and fixed a few recently introduced tests involving vectorized functions. Then ...

I guess it can be done in a separate PR from this one, but I think deprecating the vectorize_1arg etc macros themselves would also make sense

I agree, @tkelman. That is, you've already replaced all calls to @vectorize_1arg in Base with @dep_vectorize_1arg, and then add a new definition of @vectorize_1arg to deprecated.jl that prints a deprecation warning and then calls @dep_vectorize_1arg.

... added a commit deprecating @vectorize_1arg and @vectorize_2arg. Getting the deprecated calls to work without complaint and with non-mangled deprecation warnings heightened my appreciation for the objectives of #10940 (see the definitions of @dep_vectorize_1arg and @dep_vectorize_2arg). Best!

Member

Sacha0 commented Aug 13, 2016

Rebased and fixed a few recently introduced tests involving vectorized functions. Then ...

I guess it can be done in a separate PR from this one, but I think deprecating the vectorize_1arg etc macros themselves would also make sense

I agree, @tkelman. That is, you've already replaced all calls to @vectorize_1arg in Base with @dep_vectorize_1arg, and then add a new definition of @vectorize_1arg to deprecated.jl that prints a deprecation warning and then calls @dep_vectorize_1arg.

... added a commit deprecating @vectorize_1arg and @vectorize_2arg. Getting the deprecated calls to work without complaint and with non-mangled deprecation warnings heightened my appreciation for the objectives of #10940 (see the definitions of @dep_vectorize_1arg and @dep_vectorize_2arg). Best!

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 13, 2016

Member

Might require some manual updates as well?

I've added a commit that: (1) removes mentions of operation over arrays from the documentation for the formerly-macro-vectorized functions; and (2) also removes the manual subsection on @vectorize_1arg/@vectorize_2arg. Other documentation modifications that should accompany this PR? Best!

Member

Sacha0 commented Aug 13, 2016

Might require some manual updates as well?

I've added a commit that: (1) removes mentions of operation over arrays from the documentation for the formerly-macro-vectorized functions; and (2) also removes the manual subsection on @vectorize_1arg/@vectorize_2arg. Other documentation modifications that should accompany this PR? Best!

@tkelman tkelman referenced this pull request Aug 16, 2016

Merged

Airy function cleanup #18050

@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Aug 30, 2016

Member

With benefit of #18200 this passes all tests again. Anything I've missed? Thanks!

Member

Sacha0 commented Aug 30, 2016

With benefit of #18200 this passes all tests again. Anything I've missed? Thanks!

Show outdated Hide outdated base/dates/conversions.jl
@vectorize_1arg DateTime datetime2rata
@vectorize_1arg Real julian2datetime
@vectorize_1arg DateTime datetime2julian
datetime2julian(dt::DateTime) = (value(dt) - JULIANEPOCH)/86400000.0

This comment has been minimized.

@stevengj

stevengj Aug 30, 2016

Member

missing newline

@stevengj

stevengj Aug 30, 2016

Member

missing newline

Show outdated Hide outdated base/sparse/sparsematrix.jl
@@ -295,7 +295,7 @@ Convert a sparse matrix or vector `S` into a dense matrix or vector.
"""
full
float(S::SparseMatrixCSC) = SparseMatrixCSC(S.m, S.n, copy(S.colptr), copy(S.rowval), float(copy(S.nzval)))
float(S::SparseMatrixCSC) = SparseMatrixCSC(S.m, S.n, copy(S.colptr), copy(S.rowval), float.(copy(S.nzval)))

This comment has been minimized.

@stevengj

stevengj Aug 30, 2016

Member

Maybe just float.(S.nzval) here? Calling copy seems to result in two copies being made.

@stevengj

stevengj Aug 30, 2016

Member

Maybe just float.(S.nzval) here? Calling copy seems to result in two copies being made.

Show outdated Hide outdated base/special/erf.jl
erfcinv(x::Integer) = erfcinv(float(x))
@vectorize_1arg Real erfcinv
erfcinv(x::Integer) = erfcinv(float(x))

This comment has been minimized.

@stevengj

stevengj Aug 30, 2016

Member

missing newline

@stevengj

stevengj Aug 30, 2016

Member

missing newline

Show outdated Hide outdated doc/manual/arrays.rst
2×3 Array{Int64,2}:
1 4 16
25 36 49
To enable convenient vectorization of other operations, Julia provides compact syntax

This comment has been minimized.

@stevengj

stevengj Aug 30, 2016

Member
To enable convenient vectorization of mathematical and other operations
Julia provides the compact syntax `f.(args...)`, e.g. `sin.(x)` or `min.(x,y)`, for elementwise operations
over arrays or mixtures of arrays and scalars (a :func:`broadcast`) operation.  See
See :ref:`man-dot-vectorizing`:.
@stevengj

stevengj Aug 30, 2016

Member
To enable convenient vectorization of mathematical and other operations
Julia provides the compact syntax `f.(args...)`, e.g. `sin.(x)` or `min.(x,y)`, for elementwise operations
over arrays or mixtures of arrays and scalars (a :func:`broadcast`) operation.  See
See :ref:`man-dot-vectorizing`:.
@Sacha0

This comment has been minimized.

Show comment
Hide comment
@Sacha0

Sacha0 Sep 4, 2016

Member

Rebased. Thanks for the ping!

Member

Sacha0 commented Sep 4, 2016

Rebased. Thanks for the ping!

@tkelman tkelman merged commit 4533900 into JuliaLang:master Sep 4, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

vtjnash referenced this pull request Sep 5, 2016

@Sacha0 Sacha0 deleted the Sacha0:vectobroad branch Sep 10, 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 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