Documentation: Molecular Dynamics - use MD structure#145
Conversation
|
The unit test failed in what looks like an unrelated but meaningful way, but this is merged without comment. What is happening? |
|
This was changing a variable name in the documentation. The unit test fail as the MD is a bit un-predictable basically running it again usually fixes this issue. |
I would be much more comfortable if the tests passed. Despite the fact that this PR is (or at least is to our understanding) unrelated to the failure, I still think it would be best practice to rerun the tests until they pass prior to merging -- that is if we're accepting regularly failing tests to start with. Merging PRs with meaningfully failing unit tests does not look good and is potentially dangerous. We should always make sure that the tests -- at an absolute bare minimum -- can pass with new changes. |
|
Also if this is a known problem that we are just putting up with, there should be an issue about it. If you know the PR in which it became an issue, the PR should link that. It's not a good state to be in, but if we track it well enough then we're much more likely to quickly get out of the bad state. |
|
I suggest a fix in #146 - as MD is stochastic the kinetic energy follows a normal distribution so the tests fail in some occasions. By extending the test range, the tests fail less often. Still it makes sense to test the rough order of magnitude of the energy to prevent unit conversion issues and so on. |
No description provided.