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 support for AMBER as a free-energy perturbation engine #272
Conversation
It appears that all OpenMM tests are failing on Windows. Will debug tomorrow. |
Okay, it seems that I can get the correct electrostatics for No idea about the Windows failure. It passes on all other platforms. All I can think is that |
Okay, the Windows error with the OpenMM tests is: ---------------------------- Captured stdout call -----------------------------
Starting %PREFIX%\Library\bin\sire_python.exe: number of threads equals 4
None
File "C:\Users\RUNNER~1\AppData\Local\Temp\tmp0j8ahz0t/test_script.py", line 21
prm = parmed.load_file('C:\Users\RUNNER~1\AppData\Local\Temp\tmp0j8ahz0t/test.prm7', 'C:\Users\RUNNER~1\AppData\Local\Temp\tmp0j8ahz0t/test.rst7')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 2-3: truncated \UXXXXXXXX escape The tests in the Exscientia Sandpit pass since the codet doesn't use the full path to the file, so doesn't run into the unicode issue. I made some modifications to handle the new restraint files so I'll need to check the logic for the other engines too and avoid using the absolute path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are good, and can be approved.
However, I think the root cause of your unicode issue that you are constructing file paths manually. I used to do this as well, but it ended up really biting me on windows because my file paths ended up with a mixture of forward and backward slashs (as your paths do in the error). This really confuses windows, and led to lots of random behaviour, especially when sire was launched from different shells (i.e. cmd versus powershell versus whatever jupyterlab provided).
The (annoying) solution is to replace all manual path construction with calls to os.path.join
. For example, see src/sire/_load.py
(or search for this in other files).
Yes, I've used |
In reality, it would probably be best to use |
This PR adds support for using AMBER as an FEP engine, using either
pmemd
orpmemd.cuda
. This ports the code from the Exscientia sandpit into the core, making sure that it is as interoperable as possible. Thankfully this wasn't too painful since I already did the work of writing all of the AMBER output parsing in the Sandpit anyway, so could copy most straight across.The major changes are:
find_exe
function to the AMBER process to aid finding a supported AMBER executable for the requested simulation protocol. This can also handle different variants of thepmemd
executables via globbing.pmemd
andpmemd.cuda
. There are now tests that I can run locally in order to validate that single-point energies agree forsander
,pmemd
, andpmemd.cuda
. (More on this later.)Process
objects when setting up FEP simulations.somd1_compatibility
option to AMBER and GROMACS for FEP comparisons.The main challenge with the PR was getting things to work reliably with
pmemd
andpmemd.cuda
, in particular, for vacuum simulations. One thing that I hadn't realised was that the Exscientia sandpit code is actually overloaded by private internal functionality that I don't have access too, hence some configuration options (on which I was basing my port) are not what is used in practice. The code also doesn't even work as is, i.e. in the way in which a regular BioSimSpace user would interface with it (how the other engines work).With regards to differences between
pmemd
andpmemd.cuda
, the main pain is thatpmemd.cuda
doesn't support implicit solvent for vacuum simulations so you need to make sure to add a simulation box and run with a cutoff instead. (pmemd
can run with no box and an "infinite" cutoff.) From my single-point energy tests I found that I can get essentially perfect agreement in energy betweenpmemd
andpmemd.cuda
, except for vacuum FEP simulations. For my test system I find that the energy components are:pmemd
:pmemd.cuda
:From this it is clear that the only difference is in non-bonded
1-4 VDW
and1-4 EEL
terms.At first I thought that this difference was because of performing one simulation using a periodic box with a cutoff, and the other with no box and no cutoff. However, I then checked single-point energies for an ethane-->methanol merged molecule using two approaches:
BioSimSpace.Protocol.Minimisation(steps=1)
. For non-FEP simulations, bothpmemd
andpmemd.cuda
support use of implicit solvent with no box. Here the results are in agreement:pmemd
:pmemd.cuda
:BioSimSpace.Protocol.FreeEnergyMinimisation(steps=1)
. Here the results differ (as we saw earlier), but it is clear that despitepmemd.cuda
having the different setup (now using a box with cutoff) it's actually thepmemd
result that is the outlier, i.e.pmemd.cuda
does agree with the non-FEP result.pmemd
:pmemd.cuda
:Clearly something in the non-bonded calculation differs between
pmemd
andpmemd.cuda
, but only for vacuum FEP simulations. Not being an expert here, I'm wondering if there is some additional configuration option that I need to set. (I've looked around, but can't see anything.) I believe thatpmemd
was used for previous hydration free-energy calculations, e.g. some of @jmichel80's, so it would be good to know if this was an issue then? It could be that the code is broken, in which case we should probably only usepmemd.cuda
for FEP in vacuum when it's available. (I have also checked that this difference gives a difference in dG when performing a vacuum hydration free-energy leg, i.e. it doesn't somehow cancel out.) Let me know what you think.Checklist:
devel
into this branch before issuing this pull request (e.g. by runninggit pull origin devel
): [y]Suggested reviewers:
@chryswoods