Support for multiple forcefields #2
Conversation
Improvements on forcefield specification for mixed systems and specific water models
Implemented changes in comments, ran pre-commit
Did my best to address all comments
|
|
||
| def test_get_input_set_w_charges_and_forcefields(self): |
There was a problem hiding this comment.
I think we should add a more diverse set of tests and test more of the possible forcefields. This would be a great case for pytest parameterize, which would let you test a long list of forcefield inputs with almost complete code re-use.
I also think we should make sure that this is actually doing what we expect. Currently, we are just testing to make sure that it doesn't fail, but there is no confirmation that the Systems are actually parameterized differently.
We could test parameterization by instantiating a system using input_set['system.xml'].get_system() and then using the System.getForces() iterator to loop through the forces.
Even just looping through all the forces and confirming that they are different for different force fields is enough. I'd recommend testing this with just a lone water molecule and with a lone ethanol (or any organic). That'll keep the resulting Forces arrays small. Just generating the Forces and then using those values as a test would be sufficient, no need to look up in the Sage or GAFF docs what the values are supposed to be.
There was a problem hiding this comment.
Sounds great, this will take me a bit longer to get to but at the very least will have it done by next week
|
The changes look great. Thanks for implementing. I have a few more requests in the same vein and some suggestions for other tests to add. |
Added function to assign_biopolymer_and_water_ff
| charged_mols = assign_charges_to_mols( | ||
| smile_strings=smile_strings, | ||
| partial_charges=partial_charges, | ||
| partial_charge_method=partial_charge_method, | ||
| partial_charge_scaling=partial_charge_scaling, | ||
| ) |
There was a problem hiding this comment.
can we pull this out front and call it once for small molecules and biopolymer_or_water?
| @@ -0,0 +1 @@ | |||
| repo_token: KpgnfDK8BDSSTSS4lSrCvHwHnchLgIQ0D | |||
There was a problem hiding this comment.
Will this link up with a GitHub Hook? I don't quite understand how to get coveralls working, definitely a good idea though.
There was a problem hiding this comment.
This was an attempt to get it working. Still fairly confused but will try and get it to work. It's an action on materialsproject/pymatgen, and I don't understand why that isn't run here
There was a problem hiding this comment.
Ahh, that would have to be set up on my repo. That's my responsibility so for now I'd just remove this.
Later on, (read: post-qual) we'll break out all this code into a separate repo. At that point, we'll set up CI + coveralls.
Adding unit tests for: 1. smiles with formal charge 2. Ensuring different water models assign different parameters 3. Testing mixing of small molecule ff with water ff Also removed tip4p support because it requires the addition of an extra particle. Need to figure that out
Fixed smiles string in test_formal_charge Be careful about smiles!!
ran black
Improvements on forcefield specification for mixed systems and specific water models
Summary
Include a summary of major changes in bullet points:
TODO (if any)
Checklist
Work-in-progress pull requests are encouraged, but please put [WIP]
in the pull request title.