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

Use a reference of problem for set_problem() #4

Merged
merged 6 commits into from
Mar 23, 2024

Conversation

mhovd
Copy link
Contributor

@mhovd mhovd commented Mar 21, 2024

During simulation it may be necessary to update the state, which also requires the problem. Previously set_problem() consumed problem, but this change instead calls for a reference. The problem itself is small, and as such the clone should not be especially expensive.

mhovd added 3 commits March 21, 2024 14:36
During simulation it may be necessary to update the state, which also requires the problem. Previously `set_problem()` consumed `problem`, but this change instead calls for a reference. The problem itself is small, and as such the clone should not be especially expensive.
Use a reference of problem for set_problem
@mhovd
Copy link
Contributor Author

mhovd commented Mar 21, 2024

Alternatively a different solution focused on updating the state without requiring a new call to set_problem()? It feels like an XY problem.

Copy link
Owner

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mhovd, few minor changes below to reduce some lines of unnessessary code:

src/ode_solver/bdf.rs Outdated Show resolved Hide resolved
src/ode_solver/mod.rs Outdated Show resolved Hide resolved
src/ode_solver/mod.rs Outdated Show resolved Hide resolved
@martinjrobins
Copy link
Owner

Alternatively a different solution focused on updating the state without requiring a new call to set_problem()? It feels like an XY problem.

As I see it, the old solver method needs to know if the state has been altered, so it can reset its own internal state. You could either:

  • have the ode solver method own the state, so to alter the state you need to call a method on the ode solver method
  • have a reset() function on the ode solver method so it knows to reset its internal state (but keep the current problem). This will be pretty similar to the current set_problem solution, just with one less arg

@Siel
Copy link
Contributor

Siel commented Mar 22, 2024

I understand that sometimes it is necessary to update the state (and if we're planning to support events this will be happening all the time). But I'm not sure about the approach.

Wouldn't it be better if we have a sort of higher level orchestrator that sub divides the time vector to stop the solver at the event's time, and then modifies the state by changing the initial condition of the next simulation?

I know there is some overhead to this approach, namely the need to interpolate for every event time, but this approach (I think) can be more easily extrapolated when new solvers are implemented.

@martinjrobins
Copy link
Owner

Wouldn't it be better if we have a sort of higher level orchestrator that sub divides the time vector to stop the solver at the event's time, and then modifies the state by changing the initial condition of the next simulation?

Agreed, I think this PR is to sort out the api on the solver methods that this orchestrator could use to implement this.

mhovd and others added 3 commits March 23, 2024 17:27
Copy link
Owner

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks @mhovd

@martinjrobins martinjrobins merged commit 0fc656d into martinjrobins:main Mar 23, 2024
6 checks passed
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