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

replace take_state with set_state #44

Closed
martinjrobins opened this issue May 3, 2024 · 3 comments · Fixed by #52
Closed

replace take_state with set_state #44

martinjrobins opened this issue May 3, 2024 · 3 comments · Fixed by #52

Comments

@martinjrobins
Copy link
Owner

at the moment OdeEquations has the following function for take state:

/// Take the current state of the solver, if it exists, returning it to the user. This is useful if you want to use this
/// state in another solver or problem. Note that this will unset the current problem and solver state, so you will need to call
/// `set_problem` again before calling `step` or `solve`.
fn take_state(&mut self) -> Option<OdeSolverState<Eqn::V>>;

However, when you set the state again with set_problem, there is a lot of unnecessary initialisation done, e.g. setting the initial timestep (see #34 ).

Option A:

I'm thinking of replacing take_step by a set_state, which would allow a user to set the internal OdeSolverState, while keeping the initialisation of other internal variables to a minimum

fn set_state(&mut self, new_state: &Eqn::V);

So user code would be:

let mut new_state = solver.state().clone();
new_state[0] += 1.0;
solver.set_state(&new_state);

Option B:

An alternative is to keep take_step, and add set_state that takes ownership of the vector, like so

fn set_state(&mut self, new_state: Eqn::V);

So then in user code you have:

let mut state = solver.take_state().unwrap();
state[0] += 1.0;
solver.set_state(state);

This allows you to mutate the state in-place so you don't have to have two versions of the state vector

@martinjrobins
Copy link
Owner Author

I'm leaning towards option B for this so you don't have to do the clone()

@martinjrobins
Copy link
Owner Author

martinjrobins commented May 7, 2024

actually, I think this would be better:

solver.state_mut()[0] += 1.0;
solver.step();

where state_mut returns a VectorViewMut tied to the lifetime of the solver (so you can't call step without dropping state)

This would complicate the solvers, which would need to keep track if state has been mutated or not, but user code would be nicer and would reduce the size of the api

@martinjrobins
Copy link
Owner Author

I ended up keeping take_state and adding state_mut, take_state would still be useful for transferring state to another solver

martinjrobins added a commit that referenced this issue May 15, 2024
* feat: add state_mut to methods #44

* feat: generating a new state automatically does the setup

* docs: fix some docs
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 a pull request may close this issue.

1 participant