openforcefield/smirnoff99Frosst

Add parameter assignment/energy comparison tests

Opened this issue · 9 comments

Every time I do a pull request here I think it would be good practice to include some kind of integrated testing. This is related to issue #63 as once we know what we want to validate on we could include that in the tests. However, it seems like it would be good practice to at least parse the file with forcefield.py to make sure all the new parameters were added correctly.

Agreed, yes.

Possibly a test which would also work within Travis is to have a small-ish or modest set of molecules which you would energy minimize and then evaluate a GBSA solvation free energy via a single-point energy evaluation. I would think that this would allow you to catch "large geometry mistake" type errors.

(The cost of an energy minimization of a single molecule is quite inexpensive).

(You could also look at geometry errors with respect to a reference set of conformations.)

However, if we're intentionally fixing something then you expect the energy and conformations to be different. I agree that some kind of energy check would be appropriate, it is just how to handle it when we intentionally want to make changes.

Right. This would be more of a way of checking if we accidentally break something we didn't intend to change.

The test could provide a list of which molecules change/which don't and which parameters apply to them; the user could then verify that only molecules using the intended parameters have changed.

OK, so that would be more of a human intervention testing, which will be great for validation as discussed in #63. Here I was more just imagining making a Travis test that can be run before pull requests are accepted as at least an idiot check.

Yes. Basically what I'm saying is that you could have travis automatically run a test on a limited set of molecules to ensure nothing is broken; if it fails it could also provide a list of molecules for which it failed and then the person submitting the PR could verify in comments on the PR (as part of the review process) that all of those molecules in fact use the modified parameter (e.g. that it only broke things which should be broken).

If your tests operate on a by-molecule basis (and I can give an example of this) it would clearly indicate which molecules pass and fail.

So basically this would be an "idiot check plus": If it passes they haven't done anything significant, and if only a subset passes they have to manually verify the things failing are supposed to fail (e.g. by running a utility tool we could provide).

OK, that makes a bit more sense. It sounds like a good plan.

Basic "loadability" tests were added in #96

I'm changing the title of this Issue to "add parameter assignment/energy comparison tests`, since that seems to have been the substance of the discussion here.