coarse-graining / cgnet

learning coarse-grained force fields
BSD 3-Clause "New" or "Revised" License
57 stars 26 forks source link

True simulation starting point not saved #152

Closed nec4 closed 4 years ago

nec4 commented 4 years ago

This is a subtle issue, but the Simulation class does not save the initial coordinates. While the save condition is certainly fufilled for t=0, it saves the coordinates after the very first update. The effect is that the zeroth step coordinates are "jiggled" slightly, but are not the true starting coordinates. Is this the intended behavior? I propose we make a note of this in the class docs. The relevant code lines are below:

        for t in range(self.length):
            potential, forces = self.model(x_old, self.embeddings)
            potential = potential.detach()
            forces = forces.detach()
            noise = torch.randn(self.n_sims,
                                self.n_beads,
                                self.n_dims,
                                generator=self.rng).to(self.device)
            x_new = (x_old.detach() + forces*dtau +
                     np.sqrt(2*dtau/self.beta)*noise)

            if t % self.save_interval == 0:
                self.simulated_traj[t//self.save_interval, :, :] = x_new
brookehus commented 4 years ago

This is directly translated from Jiang's original code. We can save the initial coordinates as an attribute or we can make them the starts of the simulations. I might have a slight preference for the former. Presumably since the user is inputting them in the first place they're always knowable. What do you think?

brookehus commented 4 years ago

@nec4 if you want to do this differently than it currently is, now is the time for me to make the change in #162. let's discuss.

brookehus commented 4 years ago

Given that we didn't change this in the refactor I'm closing the issue. For the record my opinion is that the starting coords should not be saved in the coordinate trajectory for these reasons:

  1. they’re not a simulation output
  2. the Simulation class saves them if you want them
  3. the first step isn’t going to be that different anyway
  4. it’s weird to simulate n steps and get an n+1 sized result