More types for nodes #26

rafaqz · 2018-04-25T16:10:35Z

I needed parametric types on nodes, with different type parameters on each node. With Vectors that gives Union types.

Not for merging yet - I'll follow up with some tests. Just checking this is a reasonable idea.

ChrisRackauckas · 2018-04-25T16:46:03Z

Wouldn't this make all of the indexing not type-stable (if used)? It could be useful but it would be much slower.

rafaqz · 2018-04-25T23:50:45Z

Accessing the parametric fields with haskell-style head/tail recursive function application is type-stable in this version. But you're right - array access is now unstable! creating a big problem to solve a small one. I'll have a look, but I imagine it would require making everything recursive to fix it.

It's also possible 0.7 union types will be fast enough to fix this for small problems.

ChrisRackauckas · 2018-04-26T00:34:18Z

I'll have a look, but I imagine it would require making everything recursive to fix it.

That's fine if broadcast is defined appropriately. And I think it's parametrically-unstable: for the standard vector types it's still type-stable, but only when you put something odd in there it will not be.

It's also possible 0.7 union types will be fast enough to fix this for small problems.

Yeah, that should be fine.

This needs passing tests though.

rafaqz · 2018-04-26T01:10:47Z

I think it will still be a net gain for me because I'll have unstable types everywhere using an union vector anyway, my problem is basically the same as discussed in #5. And yes I hoped this change just makes it easier to do something slow, but should work as usual if you just use a typed Vector.

I'll get the tests passing, was saving time in case this was just a bad idea.

rafaqz · 2018-04-26T02:13:35Z

Seems this is failing on master as well - but not the same commit on tag v.0.6?

Master fails locally for me on 0.6.2.

ChrisRackauckas · 2018-04-26T02:52:27Z

Interesting. I'll have to find time to take a look at it. The code you have looks fine.

rafaqz · 2018-04-26T13:58:44Z

src/indexing.jl

@@ -25,7 +58,7 @@ end

 @inline function getindex(m::AbstractMultiScaleArray, i, I...)
    if isempty(m.values) || i < length(m.end_idxs)
-        length(I) == 1 ? m.nodes[i].nodes[I[1]] : m.nodes[i][I...]


I'm not sure what this line does? Seems like it can error in a couple of ways... The tuple version is not type stable currently

It handles the case where I is a scalar since then it cannot splat I think.

rafaqz · 2018-04-26T14:19:39Z

getindex for parametric typed tuples seems to be pretty much type stable now, except for the line mentioned above. I'll do some more comprehensive tests of type stability and some general tests and push them in the next few days.

Maybe it could be optimised more too, I'm not that clear on when @inline etc should be used.

Also there's probably a much more julian way of doing this. It looks pretty weird.

rafaqz · 2018-05-01T02:23:30Z

Indexing type stability turned out to be easier than I thought.

But the real problem turns out to be that running an ODE triggers add_node!. That means the struct would always need to be mutable, and all the nodes would need to be packed into a new tuple. I'm not sure what add_node! does in the solver?

ChrisRackauckas · 2018-05-01T02:25:04Z

But the real problem turns out to be that running an ODE triggers add_node!. That means the struct would always need to be mutable, and all the nodes would need to be packed into a new tuple. I'm not sure what add_node! does in the solver?

One of the tests is about resizing. Resizing and mutability is one of the possible behaviors. However, it's only done if the user makes an event that triggers it. It shouldn't be triggered on its own.

ChrisRackauckas · 2018-05-01T02:43:42Z

test/tuple_nodes.jl

@@ -89,7 +89,7 @@ growing_cb = DiscreteCallback(condition, affect!)

 println("ODE with tuple nodes")

-prob = ODEProblem(f, scenario, (0.0, 1.0))
+prob = ODEProblem(f, dscenario, scenario, (0.0, 1.0))


This syntax doesn't make sense. The previous one was correct.

ChrisRackauckas · 2018-05-01T02:45:02Z

The test problem you're using has an event:

tstop = [0.5]
condition = function (u, t, integrator)
    t ∈ tstop
end
affect! = function (integrator)
    add_node!(integrator, integrator.u[1, 1, 1], 1, 1)
end
growing_cb = DiscreteCallback(condition, affect!)

that tries to add a new plant at t=0.5. That's not compatible with the tuples. I assume you didn't mean to add this event?

ChrisRackauckas · 2018-05-01T03:50:24Z

src/indexing.jl

@@ -7,7 +10,7 @@ Base.IndexStyle(::Type{<:AbstractMultiScaleArray}) = IndexLinear()
 @inline function getindex(m::AbstractMultiScaleArray, i::Int)
    idx = bisect_search(m.end_idxs, i)
    idx > 1 && (i -= m.end_idxs[idx-1]) # also works with values
-    (isempty(m.values) || idx < length(m.end_idxs)) ? m.nodes[idx][i] : m.values[i]
+    (isempty(m.values) || idx < length(m.end_idxs)) ? nodeselect(m.nodes, idx, i) : m.values[i]


this is no different?

Sorry turned out that wasn't even needed after all, something else fixed the type stability. I'm still scratching my head...

rafaqz · 2018-05-01T05:37:24Z

Ok that makes sense with the event, I just copied some of your tests for arrays.

But it's good to know where using tuples will break. I will look at writing an add_node! method that replaces the whole tuple so you can use events if you use a mutable type.

rafaqz · 2018-05-02T01:38:52Z

I think my tests are all passing, the test that failing is line 67 of dinamic_diffeq.jl:

sol = solve(prob, Rosenbrock23(), callback=growing_cb, tstops=tstop)

LoadError: MethodError: no method matching resize!(::Embryo{Tissue{Population{Cell{ForwardDiff.Dual{ForwardDiff.Tag{DiffEqDiffTools.UJacobianWrapper{##27#28,Float64,Void},Float64},Float64,10}},ForwardDiff.Dual{ForwardDiff.Tag{DiffEqDiffTools.UJacobianWrapper{##27#28,Float64,Void},Float64},Float64,10}},ForwardDiff.Dual{ForwardDiff.Tag{DiffEqDiffTools.UJacobianWrapper{##27#28,Float64,Void},Float64},Float64,10}},ForwardDiff.Dual{ForwardDiff.Tag{DiffEqDiffTools.UJacobianWrapper{##27#28,Float64,Void},Float64},Float64,10}}, ::Int64)

And that's also failing on master.

ChrisRackauckas · 2018-05-08T04:30:00Z

Rebased.

ChrisRackauckas · 2018-05-08T04:45:21Z

Passing. Can this get docs?

coveralls · 2018-05-08T04:48:05Z

Coverage increased (+0.1%) to 80.357% when pulling 9c569a1 on rafaqz:loose_typed_nodes into 8f2a994 on JuliaDiffEq:master.

codecov · 2018-05-08T05:25:36Z

Codecov Report

Merging #26 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #26   +/-   ##
=======================================
  Coverage   80.23%   80.23%           
=======================================
  Files           7        7           
  Lines         167      167           
=======================================
  Hits          134      134           
  Misses         33       33

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f2a994...b14833b. Read the comment docs.

rafaqz · 2018-05-08T09:02:22Z

Yes that's a good idea. It might not be for a week or so with my current workload.

ChrisRackauckas · 2018-05-08T12:58:21Z

I went ahead and added the docs. Cheers!

rafaqz · 2018-05-09T03:26:13Z

thanks!

…

On Tue, May 8, 2018 at 10:58 PM, Christopher Rackauckas < ***@***.***> wrote: Merged #26 <#26>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACaqeendQqVfL7yKQtNAusxPa-kc94j9ks5twZZygaJpZM4TjutX> .

rafaqz commented Apr 26, 2018

View reviewed changes

ChrisRackauckas reviewed May 1, 2018

View reviewed changes

rafaqz and others added 9 commits May 7, 2018 21:28

allow AbastractVector and Tuple{Vararg} for nodes

4a6eb92

fix args...

df6cb0f

(mostly) type stable getindex for tuples

9e2de16

simpler

d4f65ea

test using tuples for nodes

3da1dfa

solver broken on add_node!

23214e5

Update tuple_nodes.jl

8fefcc0

Update tuple_nodes.jl

d11cc9c

passing tests

9c569a1

add some basic docs

b14833b

ChrisRackauckas merged commit c7bb8fe into SciML:master May 8, 2018

ChrisRackauckas mentioned this pull request Jul 17, 2018

MultiScaleModels Profiling #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More types for nodes #26

More types for nodes #26

rafaqz commented Apr 25, 2018

ChrisRackauckas commented Apr 25, 2018

rafaqz commented Apr 25, 2018 •

edited

Loading

ChrisRackauckas commented Apr 26, 2018 •

edited

Loading

rafaqz commented Apr 26, 2018

rafaqz commented Apr 26, 2018

ChrisRackauckas commented Apr 26, 2018

rafaqz Apr 26, 2018

ChrisRackauckas May 1, 2018

rafaqz commented Apr 26, 2018 •

edited

Loading

rafaqz commented May 1, 2018

ChrisRackauckas commented May 1, 2018

ChrisRackauckas May 1, 2018

ChrisRackauckas commented May 1, 2018

ChrisRackauckas May 1, 2018

rafaqz May 1, 2018

rafaqz commented May 1, 2018

rafaqz commented May 2, 2018 •

edited

Loading

ChrisRackauckas commented May 8, 2018

ChrisRackauckas commented May 8, 2018

coveralls commented May 8, 2018 •

edited

Loading

codecov bot commented May 8, 2018 •

edited

Loading

rafaqz commented May 8, 2018

ChrisRackauckas commented May 8, 2018

rafaqz commented May 9, 2018 via email

More types for nodes #26

More types for nodes #26

Conversation

rafaqz commented Apr 25, 2018

ChrisRackauckas commented Apr 25, 2018

rafaqz commented Apr 25, 2018 • edited Loading

ChrisRackauckas commented Apr 26, 2018 • edited Loading

rafaqz commented Apr 26, 2018

rafaqz commented Apr 26, 2018

ChrisRackauckas commented Apr 26, 2018

rafaqz Apr 26, 2018

Choose a reason for hiding this comment

ChrisRackauckas May 1, 2018

Choose a reason for hiding this comment

rafaqz commented Apr 26, 2018 • edited Loading

rafaqz commented May 1, 2018

ChrisRackauckas commented May 1, 2018

ChrisRackauckas May 1, 2018

Choose a reason for hiding this comment

ChrisRackauckas commented May 1, 2018

ChrisRackauckas May 1, 2018

Choose a reason for hiding this comment

rafaqz May 1, 2018

Choose a reason for hiding this comment

rafaqz commented May 1, 2018

rafaqz commented May 2, 2018 • edited Loading

ChrisRackauckas commented May 8, 2018

ChrisRackauckas commented May 8, 2018

coveralls commented May 8, 2018 • edited Loading

codecov bot commented May 8, 2018 • edited Loading

Codecov Report

rafaqz commented May 8, 2018

ChrisRackauckas commented May 8, 2018

rafaqz commented May 9, 2018 via email

rafaqz commented Apr 25, 2018 •

edited

Loading

ChrisRackauckas commented Apr 26, 2018 •

edited

Loading

rafaqz commented Apr 26, 2018 •

edited

Loading

rafaqz commented May 2, 2018 •

edited

Loading

coveralls commented May 8, 2018 •

edited

Loading

codecov bot commented May 8, 2018 •

edited

Loading