Enable PointMutationExecutor input of solvated pdbs #967
Conversation
| @@ -141,6 +144,8 @@ def __init__(self, | |||
| map of new to old sidechain atom indices to add to the default map (by default, we only map backbone atoms and CBs) | |||
| demap_CBs : bool, default False | |||
| whether to remove CBs from the mapping | |||
| solvate : bool, default True | |||
There was a problem hiding this comment.
can you clarify when solvation happens. perses does solvate protein and protein:ligand systems. i think the difference is when you are solvating, or rather, whether you have provided a pre-solvated pdb/structure file. perhaps this should change the name of the argument you are using. can you put into the docs that if False, it should be pre-solvated. Also, i was going to suggest adding a sanity check that will check if there are waters in the struct file/topology and raise an error if there are waters, but one has already specified solvate=True. Then again, if the pdb contains xtal waters that are important, that error would be erroneous. maybe adding a sanity exception error if there are lots of waters in the pdb structure.
There was a problem hiding this comment.
can you put into the docs that if False, it should be pre-solvated.
yes
Also, i was going to suggest adding a sanity check that will check if there are waters in the struct file/topology and raise an error if there are waters, but one has already specified solvate=True. Then again, if the pdb contains xtal waters that are important, that error would be erroneous.
Yes, I thought about throwing an error like this, but like you said, if there are crystal waters present, but the PDB is not actually solvated, the error would still be present, which we don't want.
| complex_positions = unit.Quantity(value=complex_positions_vec3, unit=unit.nanometer) | ||
| else: | ||
| complex_topology = ligand_topology | ||
| complex_positions = ligand_positions |
There was a problem hiding this comment.
the exception for pre-existing waters could go here i guess?
There was a problem hiding this comment.
yes, that's the whole point of the else part of this logical branch. if the topology is already solvated, we're assuming its the full complex (not just the ligand), so we don't need to combine topologies anymore.
| ionic_strength, box_dimensions) | ||
| else: | ||
| solvated_topology = topology | ||
| solvated_positions = unit.quantity.Quantity(value=np.array( |
There was a problem hiding this comment.
there might be an issue here associated with the solvate function giving a tuple of posits rather than a list, but we'll see what the test says
There was a problem hiding this comment.
the tests pass, so i don't think there is any problem with this.
|
|
||
| return solvated_topology, solvated_positions, solvated_system | ||
| return solvated_topology, solvated_positions |
There was a problem hiding this comment.
changing the returnable can be problematic. can you return the solvated system, too?
There was a problem hiding this comment.
Can you elaborate on why you think changing the returned variables is problematic? This _solvate() function is only ever called in the __init__ of PointMutationExecutor, and I made sure to properly adjust the code in the __init__ to account for the change in returned variables. Therefore, I don't think its a problem.
I moved the code for creating the system to be outside of the _solvate function (see line 269). This way, if the users specifies a presolvated PDB as input, a system can still be generated without calling _solvate.
| @@ -13,7 +13,7 @@ def test_PointMutationExecutor(): | |||
| ionic_strength=0.15 * unit.molar, | |||
| flatten_torsions=True, | |||
| flatten_exceptions=True, | |||
| conduct_endstate_validation=False, | |||
| conduct_endstate_validation=False | |||
There was a problem hiding this comment.
Why remove the trailing comma here?
There was a problem hiding this comment.
| @@ -13,7 +13,7 @@ def test_PointMutationExecutor(): | |||
| ionic_strength=0.15 * unit.molar, | |||
| flatten_torsions=True, | |||
| flatten_exceptions=True, | |||
| conduct_endstate_validation=False, | |||
| conduct_endstate_validation=False | |||
There was a problem hiding this comment.
|
Oh my, I do not know how my review got posted so many times! @zhang-ivy the only small nit I have is to keep the trailing comma, the reason it is nice to have a trailing comma is that when later you add another argument or something, the diff is cleaner since you then won't have (as part of the diff) adding a comma to the line above. |
|
@mikemhenry : Ah thanks for the clarification -- I was wondering why you had left that in. I removed it because it seemed like you left it in by accident / syntactically its unnecessary to leave an extra comma when there are no arguments after. But if you'd prefer us to keep the comma in, feel free to add a suggested commit? |
|
@dominicrufa are you happy with this PR or do you have more questions? |
Done! Adding the comma makes the diffs cleaner/more obvious on what is being added to a function call/signature, but you are correct that they don't do anything functional AST wise! |
|
@mikemhenry : Thanks! Just messaged @dominicrufa on slack and he said this is ok to merge -- please merge when you get a chance! |
Description
Allow the PointMutationExecutor to accept solvated PDBs (previously, it only accepted solute PDBs)
Motivation and context
Resolves #???
How has this been tested?
Change log