perform_step! refactor for Rosenbrock #2320
Conversation
|
@oscardssmith All the methods should be combined into one single perform_step this way right ?? |
|
Or are there certain methods which should be kept together ?? |
|
You'll want to combine all of the caches too, just using arrays of arrays in the caches instead of different variables for each k for example |
|
@ChrisRackauckas I think the recent merge caused an error here |
|
what do you mean? |
|
I ran the tests some time ago and all the errors pointed to OrdinaryDiffEqRosenbrock.jl even though I made no changes there |
|
|
|
did you rebase? |
|
no |
|
I will do that |
|
@ChrisRackauckas Can you please review the changes to see if whatever I have done so far is correct and on track ?? If any changes are needed I will make those. |
|
This is still all written out. It should turn into loops over arrays. |
|
@ChrisRackauckas I made a recent change to Rodas23WCache and Rodas3PCache. Is this how it should be done ?? Just to confirm |
|
This isn't quite right. See ExplicitRK |
I went through the explicitrk code and understood how this should be done. I will make the changes |
| else | ||
| linsolve_tmp = du + dtd3 * dT + mass_matrix * (dtC31 * k1 + dtC32 * k2) | ||
| end | ||
| for i in 2:eachindex(A) |
There was a problem hiding this comment.
I don't think this is right since eachindex of a Matrix will loop over the scalars. I think you want for i in 2:size(A, 2)
There was a problem hiding this comment.
you also probably want an @assert that size(A,1)==size(A,2)==length(k) so the compiler can know that all the sizes match up.
|
@ChrisRackauckas @oscardssmith if we change all the perform_step! implementations to have just RosenbrockCache and RosenbrockConstantCache then would we need the others ?? Or Just one perform_step! for RosenbrockCache and one perform_step! for RosenbrockConstantCache would suffice ?? |
Exactly. That's the point of the refactor. |
|
Ok got it. |
|
It's not passing tests so it can't merge? |
|
Yes. I need to work on fixing the |
|
|
|
That's unrelated. Don't worry about it. Your actual error is that you deleted the overloads required to make Rosenbrock23 work. Rebase this. |
|
okay |
|
@ChrisRackauckas How can this be dealt with ? There is some method call error here |
|
You need to be using https://github.com/SciML/PreallocationTools.jl somewhere to make the caches autodiff compatible. |
|
That's done in build_jac_config. |
For the error I mentioned in the message above?? |
There's a function |
|
@ChrisRackauckas Should I be importing |
|
@ChrisRackauckas @oscardssmith just needed some guidance on the current failures that occur in tests. I tried to figure this out but finding it a bit hard to get where things are wrong |
|
@ChrisRackauckas can you please review the errors here ?? I will fix those. |
|
Okay. will try to figure things out and fix them. i will try to get this done today. |
| @@ -1,13 +1,49 @@ | |||
| abstract type RosenbrockMutableCache <: OrdinaryDiffEqMutableCache end | |||
| abstract type RosenbrockConstantCache <: OrdinaryDiffEqConstantCache end | |||
| @@ -1,13 +1,49 @@ | |||
| abstract type RosenbrockMutableCache <: OrdinaryDiffEqMutableCache end | |||
| abstract type RosenbrockConstantCache <: OrdinaryDiffEqConstantCache end | |||
|
|
|||
| # Fake values since non-FSAL | |||
There was a problem hiding this comment.
Actually I wanted to club the constant caches into a single RosenbrockConstantCache and the names were conflicting. So I decided to keep one.
| Rosenbrock5Cache(u, uprev, dense1, dense2, dense3, du, du1, du2, k1, k2, k3, k4, | ||
| k5, k6, k7, k8, | ||
| grad_config = build_grad_config(alg, f, tf, dus[2], t) | ||
| jac_config = build_jac_config(alg, f, uf, dus[2], uprev, u, tmp, dus[3]) |
There was a problem hiding this comment.
why was this changed to 2 and 3?
There was a problem hiding this comment.
actually dus had three variables du, du1, and du2 so I kept them in single vector as [du, du1, du2] according to Julia's 1-based indexing du1 would be dus[2] so I went that way
|
|
||
| # Calculate the final results for k | ||
| for j in 1:3 | ||
| tmp = sum(eval(Symbol("h$j$i")) * ks[i] for i in 1:8) |
There was a problem hiding this comment.
no eval allowed. it should probably be an array anyways.
|
Lots of comments. You really need to fix the merge commit before trying to continue. You cannot just delete all changes that have happened, that's going to cause test failures. There is a naming overlap with one of the new abstract types, you'll want to actually handle that and not just revert it. |
Okay. Will keep that in mind. working towards that. Sorry for the mistakes made |
|
I highly recommend moving this PR to a branch so you can keep a clean master. This was causing you issues before, and now it's interfering with the other PR. Generally, don't open PRs from your fork's master. |
ParamThakkar123
force-pushed
the
master
branch
from
5252290 to
a006653
Compare
ParamThakkar123
force-pushed
the
master
branch
from
a006653 to
eb36500
Compare
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.
#233