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

Documentation: Molecular Dynamics - use MD structure #145

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

jan-janssen
Copy link
Member

No description provided.

@jan-janssen jan-janssen merged commit 16e6c70 into main Dec 17, 2023
15 of 17 checks passed
@jan-janssen jan-janssen deleted the doc_correction branch December 17, 2023 21:54
@liamhuber
Copy link
Member

The unit test failed in what looks like an unrelated but meaningful way, but this is merged without comment. What is happening?

@jan-janssen
Copy link
Member Author

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.

@jan-janssen jan-janssen changed the title Molecular Dynamics - use MD structure Documentation: Molecular Dynamics - use MD structure Dec 17, 2023
@liamhuber
Copy link
Member

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.

@liamhuber
Copy link
Member

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.

@jan-janssen
Copy link
Member Author

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.

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.

2 participants