Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support wet octanol solvent box and AMBER/CHARMM forcefields #103

Merged
merged 45 commits into from
Aug 2, 2019

Conversation

VOD555
Copy link
Contributor

@VOD555 VOD555 commented Mar 5, 2019

Issue #98
Changes made in this Pull Request:

  • Support wet octanol solvent type.
  • Add CHARMM and AMBER octanol boxes and itp files. CHARMM parameters for octanol are generated with cgenff, and AMBER parameters are generated with gaff. All boxes were generated after a 40ns NPT equilibrium simualtion.
  • Support AMBER99sb and CHARMM36 forcefields.
  • Add tests for wet octanol box and AMBER, CHARMM boxes in test_forcefields.py.

Tests passed locally.
It seems we need test run for solvation free energy calcualtion with new forcefields. And current scripts don't support new forcefields.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?

@VOD555 VOD555 requested a review from orbeckst March 5, 2019 08:16
@VOD555
Copy link
Contributor Author

VOD555 commented Mar 5, 2019

@orbeckst @iorga There are some problems on the wet octanol box. GROMACS cannot automatically update the toplogy file when given a moxied solvent box. My idea is to count the number of ocanol molecules and manually edit topology file. Do you have any better idea?

@iorga
Copy link
Collaborator

iorga commented Mar 5, 2019

@orbeckst @iorga There are some problems on the wet octanol box. GROMACS cannot automatically update the toplogy file when given a moxied solvent box. My idea is to count the number of ocanol molecules and manually edit topology file. Do you have any better idea?

I'm fine with this solution if it solves the problem.

@orbeckst
Copy link
Member

orbeckst commented Mar 5, 2019

I don't have a better idea – I didn't realize that Gromacs only deals gracefully with single-molecule solvents.

@orbeckst
Copy link
Member

orbeckst commented Mar 5, 2019

@VOD555 ping me when you think this is ready to be reviewed or if you need specific feedback.

@orbeckst
Copy link
Member

orbeckst commented Mar 7, 2019

@VOD555 , does this PR supersede PR #90 ?

@VOD555
Copy link
Contributor Author

VOD555 commented Mar 7, 2019

@orbeckst Yes, I think that should be closed.

@orbeckst
Copy link
Member

orbeckst commented Mar 7, 2019

Do you know why the tests fail?

@orbeckst
Copy link
Member

orbeckst commented Mar 7, 2019

PR #90 contains a few things that might be useful, such as a C36 benzene itp a60e5ad for testing. I'll close the other PR but we should not forget the work that you've already done there.

@orbeckst orbeckst mentioned this pull request Mar 7, 2019
@VOD555
Copy link
Contributor Author

VOD555 commented Mar 7, 2019

Do you know why the tests fail?

Not very sure...
The tests passed on my laptop.

@orbeckst
Copy link
Member

Note: To test wet octanol we need new solvate() function in GW (devel). Make release of GW or install dev version of GW.

@orbeckst
Copy link
Member

To have MDA in the tests, it also needs to be installed in travis.yml.

@orbeckst
Copy link
Member

For the template failures: try using pip install . instead of pip install -e ..

@orbeckst
Copy link
Member

orbeckst commented Apr 4, 2019

@orbeckst : add MDAnalysis to PR

@VOD555
Copy link
Contributor Author

VOD555 commented Jul 31, 2019

@orbeckst Gromacs2019 can recognize different solvent molecules and add them to the top file, but the number of the solvent whose name is not sol would be wrong. However the old version cannot add more than one solvent to the top file. So the template top file needs to be different for new and old version of gromacs. Can we do this in MDPOW? Or we only support new version?

@orbeckst
Copy link
Member

Not sure, what do the two different templates look like? In principle we can use gromacs.release() to figure out which version of Gromacs is used.


[ molecules ]
; Compound #mols
DRUG 1
Copy link
Contributor Author

@VOD555 VOD555 Jul 31, 2019

Choose a reason for hiding this comment

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

@orbeckst The old version GROMACS needs one line OcOH 1 after the DRUG line here.

Copy link
Member

Choose a reason for hiding this comment

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

We can use Gromacs preprocessor directives in the file and define a variable MDPOWPREGMX2019

DRUG               1
#ifdef MDPOWPREGMX2019
OcOH               1
#endif

You would then add to the MDP file

define  = -DMDPOWPREGMX2019

In this way, we have the same top file and just switch how it is read by Gromacs depending on the version. MDPOW will have to add the appropriate define option when it generates the MDP file(s).

Copy link
Member

Choose a reason for hiding this comment

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

I think that this will work but perhaps try it out manually first.

@orbeckst
Copy link
Member

orbeckst commented Jul 31, 2019

@VOD555 How about get this PR done first for Gromacs 2018 and older (you can mark the test as an expected failure if gromacs.release().startswith('2019') or something similar – there's a pytest decorator). Then open a new issue/PR to update for 2019 – possibly using preprocessort constructs as in #103 (comment). That's much cleaner than fixing even more things in this PR.

@VOD555 VOD555 self-assigned this Aug 1, 2019
@codecov
Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #103 into develop will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #103   +/-   ##
======================================
  Coverage        0%     0%           
======================================
  Files           10     10           
  Lines         1214   1260   +46     
  Branches       143    143           
======================================
- Misses        1214   1260   +46
Impacted Files Coverage Δ
mdpow/equil.py 0% <0%> (ø) ⬆️
mdpow/fep.py 0% <0%> (ø) ⬆️
mdpow/forcefields.py 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c274058...a95f48e. Read the comment docs.

@VOD555
Copy link
Contributor Author

VOD555 commented Aug 1, 2019

This PR only supports wetoctanol with GROMACS version below 2018.1.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

lgtm, thank you for this major effort, @VOD555

Raise new issues as necessary, please.

@orbeckst
Copy link
Member

orbeckst commented Aug 1, 2019

@VOD555 could you please clean up and condense the history with

git rebase -i develop
# do interactive rebase
git push -f

Squash all these try this/try that commits together. What I want to avoid is having the force field files and the code changes in one commit. At a minimum 5662554 should remain a separate commit – if you want to squash everything else together then please do. Or group commits into logical units. In any case, I'd like to see a cleaner commit history.

Once this is done, we can do a simple merge.

@orbeckst
Copy link
Member

orbeckst commented Aug 1, 2019

ALSO: please add entry to CHANGELOG!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants