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 broadcast_(get|set)index in favor of broadcasted (get|set)index #27075

Merged
merged 2 commits into from
May 14, 2018

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented May 11, 2018

Here's another simple deprecation I'd like to sneak in.

The performance here is comparable in my few quick tests, and only two packages used either of these functions -- both in situations where they would have gained much more by participating in dot-fusion.

julia> A = rand(5000,5000);
julia> @benchmark broadcast_getindex($A, 1:5000, 1:5000)
BenchmarkTools.Trial:
  memory estimate:  39.14 KiB
  allocs estimate:  2
  --------------
  minimum time:     34.535 μs (0.00% GC)
  median time:      35.510 μs (0.00% GC)
  mean time:        36.835 μs (1.72% GC)
  maximum time:     2.742 ms (97.61% GC)
  --------------
  samples:          10000
  evals/sample:     1

julia> @benchmark getindex.(($A,), 1:5000, 1:5000)
BenchmarkTools.Trial:
  memory estimate:  39.14 KiB
  allocs estimate:  2
  --------------
  minimum time:     31.564 μs (0.00% GC)
  median time:      32.596 μs (0.00% GC)
  mean time:        35.887 μs (2.17% GC)
  maximum time:     3.604 ms (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

julia> @benchmark broadcast_setindex!($A, 1:5000, 1:5000, 1:5000)
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     32.503 μs (0.00% GC)
  median time:      33.403 μs (0.00% GC)
  mean time:        33.432 μs (0.00% GC)
  maximum time:     136.444 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

julia> @benchmark setindex!.(($A,), 1:5000, 1:5000, 1:5000)
BenchmarkTools.Trial:
  memory estimate:  39.14 KiB
  allocs estimate:  2
  --------------
  minimum time:     35.955 μs (0.00% GC)
  median time:      37.788 μs (0.00% GC)
  mean time:        39.442 μs (2.17% GC)
  maximum time:     642.989 μs (80.10% GC)
  --------------
  samples:          10000
  evals/sample:     1

…ndex

The performance here is comparable in my few quick tests, and only two packages used either of these functions -- both in situations where they would have gained much more by participating in dot-fusion.
@mbauman mbauman added broadcast Applying a function over a collection deprecation This change introduces or involves a deprecation labels May 11, 2018
@JeffBezanson
Copy link
Member

So nice!!

@Sacha0
Copy link
Member

Sacha0 commented May 12, 2018

CI suggests that doc references to broadcast_getindex and broadcast_setindex! need excising?

@mbauman
Copy link
Member Author

mbauman commented May 14, 2018

FreeBSD CI failure looks unrelated and is flagged as a low-probability failure.

@mbauman mbauman merged commit 66c7b7d into master May 14, 2018
@mbauman mbauman deleted the mb/deprecate-broadcast_{get,set}index branch May 14, 2018 18:30
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 deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants