Repository navigation
Toy Dynamics should have separate System object #73
Description
Per discussion in #65 (comment) ff., Toy Dynamics should have a separate object (perhaps called ToySystem or ToySystemState), such that for simulation = ToySimulation(), instead of
vel = simulation.velocities
pos = simulation.positionswe have
vel = simulation.current_system.velocities
pos = simulation.current_system.positionsThis makes Toy Dynamics more pedagogically useful
Activity
jhprinz commentedon Nov 14, 2014
I am not 100% sure what you mean. You are not saying that toy_dynamics should avoid using snapshots? So I assume that current_system means a particular simulation out of many?
If so, I would rather use several simulation objects.
dwhswenson commentedon Nov 14, 2014
Sorry for being unclear; this is just meant to address the "best practices" conversation from #65, especially so that ToyDynamics actually implements the four objects enumerated in #65 (comment). I think we agreed that this 4-object model was a best practice, though we only wanted to force the users to implement a minimal interface. However, we should use that 4-object model in our own things (unless there's a good reason not to), so that we set a good example for people learning from our code. In particular, ToyDynamics really must use the 4-object model, because there's not a single good reason for it not to.
Currently, ToyDynamics uses a 3-object model, which objects 3 and 4 from the 4-object model (simulation state and overall simulator) are lumped together. This works, but is not the most pedagogically useful approach. (The entire reason I did it that way was laziness, which does not count as a good reason.) This issue is just intended to remind me to bring ToyDynamics into accord with the 4-object model.
Internally, ToySimulation does not use
Snapshots because it doesn't have any concept of atoms. It sees the entire position array as a single vector moving on a pre-defined global potential energy surface (as opposed to a PES defined by interaction potentials).In OpenMM terms,
ToySimulation().current_system(maybe better to call itcurrent_system_state?) is intended to be likeopenmm.app.simulation.context.If simultaneously running multiple trajectories (with everything else the same), I think it makes sense to have multiple simulation objects. I think we're discussing this idea in #65, so I'll put my comments on it there.
dwhswenson commentedon Nov 14, 2014
As mentioned in #65 (comment), when this gets implemented, I should also make sure that all the information which needs to be stored about the simulation goes into its
__dict__.jhprinz commentedon Feb 7, 2015
What is the status on this. Do we still need to make changes here?
dwhswenson commentedon Feb 7, 2015
Milestoned future. It should be done, but not in 1.0.
jhprinz commentedon Mar 4, 2016
I think we can close this as it is replaced by using features and the new Snapshot factory.
dwhswenson commentedon Mar 5, 2016
No, that misses the intent. See #73 (comment).
jhprinz commentedon Mar 6, 2016
Thanks for reminding me.