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

Add liquid box creation and simulation files #149

Merged
merged 3 commits into from
Jan 1, 2019
Merged

Conversation

leeping
Copy link
Contributor

@leeping leeping commented Dec 15, 2018

This PR adds files and scripts for creating a liquid box and running a SMIRNOFF simulation on it, starting from just a SMILES string for a single molecule of the liquid. Example included for ethanol.

@hjuinj
Copy link
Collaborator

hjuinj commented Dec 16, 2018

I also wrote somethings similar using the python packmol handles John developed in openmoltools here

@davidlmobley
Copy link
Contributor

@hjuinj - yeah, I added those (or at least some of them) for use in solvationtoolkit (github.com/mobleylab/solvationtoolkit). I haven't extended that for SMIRNOFF yet but I should.

Added some formatting to the markdown in the readme file.
- OpenEye tools (for creating molecule from SMILES and conformer generation)
- openmoltools (OpenEye conformer generation wrapper)
- Gromacs 4.6.7 (for calling genbox to create solvent box)
- ForceBalance 1.5.x (for putting information back that was thrown away by genbox)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal to have ForceBalance and GROMACS dependencies, but probably OK for the time being.

float
Length of a cubic solvent box in nm.
"""
# Calculate total mass of the box in kg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually what about just using the approximate_volume_by_density function in openmoltools instead? openmoltools is already required here. Reduces code redundancy.

Copy link
Contributor

@davidlmobley davidlmobley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not necessarily opposed to merging this, but for the record I think it could be made a lot simpler by using SolvationToolkit to build the boxes (which can handle arbitrary solvents) and then the rest of the code could be considerably more concise I think, e.g. see the end of this Jupyter notebook I'd put together for a workshop in Chile: https://github.com/QCMM/workshop2017/blob/29b9f58046e4e85daee816995fb5b6944a333106/Free_energy_day1/OpenMM/Session2.ipynb

Still, maybe this is worth including as an example for now.

@leeping how about if we included this as an example for now and I put some notes in the README.md about how it could be simplified and/or dependencies reduced? Would that work for you?

@jchodera
Copy link
Member

It's fine to merge for now, I bet, but this will take more work to update for the topology branch as well.

1 similar comment
@jchodera
Copy link
Member

It's fine to merge for now, I bet, but this will take more work to update for the topology branch as well.

Copy link
Contributor

@davidlmobley davidlmobley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leeping did you see my comments? We can merge this now if you want, just wanted to make sure you saw them.

@davidlmobley
Copy link
Contributor

Will merge once @leeping acknowledges.

@leeping
Copy link
Contributor Author

leeping commented Jan 1, 2019

I agree with all of your comments and we can proceed with the merge. There are certainly ways to accomplish the same thing with fewer dependencies that we should add in the future.

@davidlmobley davidlmobley merged commit bf51eb6 into master Jan 1, 2019
@andrrizzi andrrizzi deleted the liquid_box branch May 30, 2019 16:36
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.

4 participants