Skip to content

[WIP] Rework broadcast - #41

Merged
ChrisRackauckas merged 1 commit into
SciML:masterfrom
vchuravy:vc/broadcast
Oct 10, 2018
Merged

ChrisRackauckas merged 1 commit into
SciML:masterfrom
vchuravy:vc/broadcast

Conversation

@vchuravy

Copy link
Copy Markdown
Contributor

I think we can do away with must of the generated functions,
but as they say the proof is in pudding.

Chris, is there a set of benchmarks I should test this against?

Comment thread src/array_partition.jl
@@ -72,50 +70,32 @@ Base.ones(A::ArrayPartition, dims::NTuple{N,Int}) where {N} = ones(A)

for op in (:+, :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Base got rid of all of these: do you think it is better to just delete these methods?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes absolutely.

@ChrisRackauckas

ChrisRackauckas commented Sep 26, 2018 •

Copy link
Copy Markdown
Member

Took awhile to understand but it looks good. I'm going to have to use this other places :). What's the reason for this being labelled WIP?

@ChrisRackauckas

Copy link
Copy Markdown
Member

For benchmarks it would be good to retest SciML/OrdinaryDiffEq.jl#122 (comment)

@vchuravy

Copy link
Copy Markdown
Contributor Author

What's the reason for this being labelled WIP?

I was waiting on you to review and I would have this see some more testing. Developing this was helpful since it pointed out issues in DistributedArrays implementation that I am working on fixing, and the example we discussed doesn't work at all for DArrays yet.
So I want to make sure that works before we merge this.

@ChrisRackauckas

ChrisRackauckas commented Sep 26, 2018 •

Copy link
Copy Markdown
Member

Update of SciML/OrdinaryDiffEq.jl#122 :

using DiffEqBase, OrdinaryDiffEq, RecursiveArrayTools, DiffEqDevTools
using BenchmarkTools

u0 = zeros(100)
v0 = ones(100)
f1 = function (du,v,u,p,t)
  du .= v
end
f2 = function (dv,v,u,p,t)
  dv .= -u
end
function (::typeof(f2))(::Type{Val{:analytic}}, x, y0)
  u0, v0 = y0
  ArrayPartition(u0*cos(x) + v0*sin(x), -u0*sin(x) + v0*cos(x))
end

prob = DynamicalODEProblem(f1,f2,u0,v0,(0.0,5.0))

@benchmark solve(prob,RK4(),dt=1/2,adaptive=false,save_everystep=false)

function f(du,u,p,t)
    uu = @view u[1:100]
    v = @view u[101:end]
    du[1:100] .= uu
    du[101:200] .= v
end
u02 = [zeros(100);ones(100)]
tspan=(0.0,5.0)
prob = ODEProblem(f,u02,tspan)
@benchmark sol = solve(prob,RK4(),dt=1/2,save_everystep=false)
BenchmarkTools.Trial: 
  memory estimate:  70.98 KiB
  allocs estimate:  505
  --------------
  minimum time:     47.131 μs (0.00% GC)
  median time:      53.279 μs (0.00% GC)
  mean time:        69.464 μs (8.20% GC)
  maximum time:     2.615 ms (93.94% GC)
  --------------
  samples:          10000
  evals/sample:     1

BenchmarkTools.Trial: 
  memory estimate:  149.80 KiB
  allocs estimate:  513
  --------------
  minimum time:     67.330 μs (0.00% GC)
  median time:      75.821 μs (0.00% GC)
  mean time:        97.848 μs (10.04% GC)
  maximum time:     2.486 ms (94.94% GC)
  --------------
  samples:          10000
  evals/sample:     1

Wow. It's looking pretty fantastic now performance-wise especially considering the fact that there's still a broadcast performance bug for Array{Float64} which might be involved.

Tests on OrdinaryDiffEq.jl seem to pass. I say seem to because there's something that I think is unrelated that I'm tracking down. Those tests put everything through the gauntlet so it's pretty safe if they pass. I'll make sure OrdinaryDiffEqExtendedTests.jl gets ran as well.

@ChrisRackauckas

Copy link
Copy Markdown
Member

Yeah, everything passes and the benchmarks look good.

@ChrisRackauckas
ChrisRackauckas merged commit e0567b6 into SciML:master Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants