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

Perform_step! refactor for Rosenbrock part 2 #2448

Merged
merged 5 commits into from
Sep 19, 2024

Conversation

ParamThakkar123
Copy link
Contributor

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.
solves a part of #233

@ParamThakkar123 ParamThakkar123 changed the title Fixes9 Perform_step! refactor for Rosenbrock part 2 Sep 1, 2024
@ParamThakkar123
Copy link
Contributor Author

Okay. Working on the changes

@oscardssmith
Copy link
Contributor

the interpolates still need some fixing.

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith what can be a possible fix here ?? merge the constant caches of rodas4 and rodas5 ??

@ParamThakkar123
Copy link
Contributor Author

@ChrisRackauckas What changes do I need to make to the interpolant parts exactly ?? What can be a possible fix here ??

@oscardssmith
Copy link
Contributor

You should merge the constant caches as well, and then you need to change the ode_interpolant code to look at the alg order rather than the cache type (and ideally also be generic over alg order)

@ParamThakkar123
Copy link
Contributor Author

got it.

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith I think there is a slight discrepancy with respect to the size of the vectors and matrices in Rodas4 and Rodas5. When I was merging the two the sizes of vectors and matrices in case of Rodas5 was 1 more than those in rodas4. The tests failing here seem to point to the same thing. For one solver the size of the vectors and matrices is appropriate while for the other solver it's smaller by one.

@oscardssmith
Copy link
Contributor

The Rodas5 matrices should all be bigger than those for Rodas4. If you get them to be the right size, however the math should all work out.

@ParamThakkar123
Copy link
Contributor Author

The Rodas5 matrices should all be bigger than those for Rodas4. If you get them to be the right size, however the math should all work out.

Yeah. I think they should be brought to the same sizes in such a way that, the math that goes inside is not affected. But what values can be added ?? 1 or 0 ??

@oscardssmith
Copy link
Contributor

oscardssmith commented Sep 4, 2024

You accidentally had added the last stage of Rodas5 to Rodas5P. Deleting that error (and adding the last stage that you were missing) fixes it.

@ParamThakkar123
Copy link
Contributor Author

image

@oscardssmith What changes must be made here ? The size of the k vector is found to be 2 but it accesses the 3rd element. and I didn't find where the k is defined other than as a parameter.

@ParamThakkar123
Copy link
Contributor Author

image

@oscardssmith What changes must be made here ? The size of the k vector is found to be 2 but it accesses the 3rd element. and I didn't find where the k is defined other than as a parameter.

do I need to add some check for this ?? @oscardssmith

@oscardssmith
Copy link
Contributor

the problem is you're mashing together the 4th and 5th order interpolants, You need to make this method check the order and do the appropriate interpolation (see the 4th order ones higher up in the file).

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith and just one more question (sorry for so many question, just trying to understand everything well), any algorithm has two cache, the normal one and the constant cache. How they should be handled ?? because algorithm is same, so will be the alg_order, but caches and their corresponding interpolants are different at some places

@oscardssmith
Copy link
Contributor

the interpolants are the same. The difference is just whether it's using an in place or out of place computation.

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith anything left on this PR to be done ?? I saw only precision based errors where the tests are failing

@oscardssmith
Copy link
Contributor

It's not yet ready to merge, but I think I can take it from here. There is something fascinating going wrong with Rodas5. The thing that makes very little sense to me at this point is that from what I can see, the problem only exists for in place Rodas5 (but not out of place Rodas5 or in place Rodas5P...)

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith meanwhile, should I start fixing the other solvers ?

@oscardssmith
Copy link
Contributor

If you want to make another PR for the 3rd order rosenbrock methods that would be great (make the PR on top of this one)

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith a new PR started for refactor of 3rd order rosenbrock #2463

@ParamThakkar123
Copy link
Contributor Author

ParamThakkar123 commented Sep 17, 2024

@oscardssmith any fixes to make here ??

@oscardssmith
Copy link
Contributor

I think I need to talk with @ChrisRackauckas tomorrow about some of the details of this PR.

@oscardssmith oscardssmith merged commit f17e149 into SciML:master Sep 19, 2024
56 of 60 checks passed
@oscardssmith oscardssmith removed their request for review September 19, 2024 18:11
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