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

Remove ODEProblem #27

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

Remove ODEProblem #27

wants to merge 3 commits into from

Conversation

SebastianM-C
Copy link
Contributor

This PR removes the ODEProblem code path since it was not needed
and it made algorithm support for run_simulation unnecesarly complicated (see #26).
Also, the ODEProblem code did not support thermostats from what I can tell.

Since the ODEProblem dispatch was used for the default algorithm choice with Tsit5
as the default, this changes to VelocityVerlet with a timestep equal to tspan/10000.
I opted for VelocityVerlet since it is a very popular algorithm for N-body problems.
The disadvantage compared with the previous default is that it increases runtimes
with about ~2 orders of magnitude, but the energy and other invariants of the
system should be conserved better. Maybe after the benchmarks we can choose a
better default.

I also adapted the indexing in some tests to match with the new solution type
and added some comments explaining the tests (what we test from the physics point of view).

Add some explanations for tests and remove extra whitespace
@ChrisRackauckas
Copy link
Member

Note Tsit5 works on DynamicalODEProblem, so it can still be used, but indeed it shouldn't be default in these cases. The default should be chosen based on benchmarks.

However, I don't think a default dt makes sense if an algorithm isn't adaptive.

@SebastianM-C
Copy link
Contributor Author

I put the dt because otherwise we can't have a simple run_simulation(simulation) method as shown in the README. Should we use an adaptive algorithm by default in this case?

@ChrisRackauckas
Copy link
Member

I think dt should be a required keyword argument if we use a non-adaptive default.

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