ImplictiEulerExtrapolation parallel #872
Conversation
Codecov Report
@@ Coverage Diff @@
## master #872 +/- ##
==========================================
- Coverage 80.16% 80.13% -0.03%
==========================================
Files 92 92
Lines 30269 30289 +20
==========================================
+ Hits 24265 24272 +7
- Misses 6004 6017 +13
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #872 +/- ##
==========================================
- Coverage 80.16% 80.12% -0.05%
==========================================
Files 92 92
Lines 30269 30328 +59
==========================================
+ Hits 24265 24300 +35
- Misses 6004 6028 +24
Continue to review full report at Codecov.
|
saurabhkgp21
changed the title
| @@ -392,6 +392,57 @@ function calc_W!(integrator, cache::OrdinaryDiffEqMutableCache, dtgamma, repeat_ | |||
| return nothing | |||
| end | |||
|
|
|||
| function calc_W!(integrator, cache::OrdinaryDiffEqMutableCache, dtgamma, repeat_step, W_index::Int, W_transform=false) | |||
| @unpack t,dt,uprev,u,f,p = integrator | |||
| @unpack J,W = cache | |||
There was a problem hiding this comment.
this is just silly. Why not just pass in W into these routines? It would fix these problems.
There was a problem hiding this comment.
@kanav99 @huanglangwen this is something we should follow up on. It's because our original calc_W! was never intended for thread-safety, and so to make it thread-safe there is this code duplication. We should assume this case can happen in the next refactor.
There was a problem hiding this comment.
Does it mean calc_W! should have an additional parameter of readonly W ?
There was a problem hiding this comment.
it should have J and W I think, where it writes into W for a given J, instead of pulling them from the cache (since there may be more than 1)
There was a problem hiding this comment.
But it won't work on OOP.
There was a problem hiding this comment.
For OOP it will need to return W.
There was a problem hiding this comment.
I think we should have a pseudo-nlsolver kind of thing for algorithms which don't have an actual nlsolver like Rosenbrocks, the struct should have aliases to the J and W and similar stuff, should be <: NLSolver so that we have a common implementation of the functions. This way it won't make us change the original derivative utilities everytime we make a new algorithm just for an overload.
There was a problem hiding this comment.
Note that algorithms with nlsolver struct are already well handled for parallel applications.
|
I'll merge because this is correct, but it should become a lot cleaner. However, @kanav99 is going through the W calculations right now for DAEs, so he might want to handle that transition to passing in |
| startIndex = (i==1) ? 1 : max_order | ||
| endIndex = (i==1) ? max_order-1 : max_order | ||
| for index in startIndex:endIndex | ||
| dt_temp = dt/(2^(index-1)) # Romberg sequence |
| u_tmps = Array{typeof(u_tmp),1}(undef, Threads.nthreads()) | ||
|
|
||
| for i=1:Threads.nthreads() | ||
| u_tmps[i] = zero(u_tmp) |
There was a problem hiding this comment.
one improvement that here is just to make the first ones in the arrays equal, i.e. u_tmps[1] = u_tmp. Then the algorithm would be fine. Right now you have an extra thing for each one, so you have an extra Jacobian, and that's really bad memory-wise.
|
#875 explains the cache cleanup I was mentioning with some code. |
No description provided.