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 Tsit #2431

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

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 Fixes7 Perform step refactor for Tsit Aug 25, 2024
@oscardssmith
Copy link
Contributor

how does the performance compare?

@ChrisRackauckas
Copy link
Member

Explicit RK is a bit tougher. It has a lot harder performance-wise. It needs a good understanding of #233 (comment)

@ParamThakkar123
Copy link
Contributor Author

Okay. I will refer that comment

@ParamThakkar123
Copy link
Contributor Author

@ChrisRackauckas Just needed your review on this. All the tests that have failed show some sort of precision and dimension mismatch.

@ChrisRackauckas
Copy link
Member

I don't understand the point of this PR. As mentioned in the other threads and the comment above, the general tableau for ERK methods is about feature-completeness and correctness of ExplicitRK and subsuming other RKs into that.

@ParamThakkar123
Copy link
Contributor Author

I don't understand the point of this PR. As mentioned in the other threads and the comment above, the general tableau for ERK methods is about feature-completeness and correctness of ExplicitRK and subsuming other RKs into that.

Actually, all I did here is refactor of perform_step implementations focusing only on the tsit solvers. The Tsit tableau was implemented in the library for this solvers. So I just refactored it that way. Focusing only on this.

@ChrisRackauckas
Copy link
Member

Yes, that does not make sense given ExplicitRK exists.

@ParamThakkar123
Copy link
Contributor Author

Yes, that does not make sense given ExplicitRK exists.

So should I be doing ExplicitRK first and then combining tsit into it ??

@ChrisRackauckas
Copy link
Member

ExplicitRK exists and has a Tsit5 tableau already. Its mostly about feature-completeness and performance now.

@ParamThakkar123
Copy link
Contributor Author

So what should be done further as far as this PR is concerned ?

@ParamThakkar123
Copy link
Contributor Author

@ChrisRackauckas should I move on with the refactoring of ExplicitRK first ??

@ChrisRackauckas
Copy link
Member

No it's already "refactored". It's about improving it and feature completing it. Testing it against the other implementations of RKs and making sure it floating point matches, and making sure default controller values are set appropriately.

@ParamThakkar123
Copy link
Contributor Author

ParamThakkar123 commented Aug 27, 2024

No it's already "refactored". It's about improving it and feature completing it. Testing it against the other implementations of RKs and making sure it floating point matches, and making sure default controller values are set appropriately.

Got it. Should I work towards feature completing it? What should be the first thing that should be done to start off with that ?

@ChrisRackauckas
Copy link
Member

Run Tsit5 and the tableau form of Tsit5 and get the times array to match exactly. I think the controller params are probably the place to start.

@ParamThakkar123
Copy link
Contributor Author

Run Tsit5 and the tableau form of Tsit5 and get the times array to match exactly. I think the controller params are probably the place to start.

@ChrisRackauckas I ran the Tsit5 and its tableau form on a sample DE as follows :
image

and the times arrays seem to match exactly

@ChrisRackauckas
Copy link
Member

What about with stiffness detection and switching?

@ParamThakkar123
Copy link
Contributor Author

ParamThakkar123 commented Aug 28, 2024

What about with stiffness detection and switching?

image

code :
image

@ChrisRackauckas

@ChrisRackauckas
Copy link
Member

I mean build an autoswitch alg that uses the ExplicitRK version, does that match the Tsit5 behavior?

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.

3 participants