add small molecule charge change support with test #973

dominicrufa · 2022-03-21T19:59:46Z

Description

add small-molecule charge change support and attempt to generalize the existing protein mutation charge change functionality to avoid code duplication

Motivation and context

adds this support

How has this been tested?

performs a RelativeFEPSetup on two Bace ligands with unequal charge, performs energy validation and asserts both systems are neutral after modification

zhang-ivy · 2022-03-22T15:43:29Z

@dominicrufa : Looks like the tests are failing for this -- can you fix the errors and make sure you run the relevant tests locally with pytest before pushing again?

zhang-ivy · 2022-03-22T16:00:11Z

I just reviewed. It mostly looks good, but I find the locations of all the helper functions confusing:

_get_charge_difference(current_oemol, new_oemol) and get_water_indices(charge_diff, new_positions, new_topology, radius=0.8): are present in SmallMoleculeSetProposalEngine
_get_charge_difference(current_resname, new_resname) is in PolymerProposalEngine
modify_atom_classes(water_atoms, topology_proposal): is just placed at the bottom of topology_proposal.py
get_ion_and_water_parameters(system, topology, positive_ion_name="NA", negative_ion_name="CL", water_name="HOH") and transform_waters_into_ions(water_atoms, system, charge_diff, particle_parameter_dict) are in perses/utils/smallmolecules.py

Why don't we just place all these helper functions in a new file called utils/charge_changing.py (or something like this)? None of the helper functions actually require any SmallMoleculeSetProposalEngine/PolymerProposalEngine class attributes, so they can be separated easily. I also don't think it makes sense to require that the PointMutationExecutor import SmallMoleculeSetProposalEngine , as would be required by the current implementation.

jchodera · 2022-03-22T22:29:05Z

One other thing to note here is that once biopolymer support is added to OpenFF, we can take another pass at overhauling and simplifying this code substantially: We will simply be able to compute the total charge of the Molecule or Topology and adjust accordingly. So it may not be worthwhile to spend too much time refactoring at this stage.

jchodera · 2022-04-01T15:21:43Z

@dominicrufa : Just a reminder that we'd love to finish this up if possible!

…nd refactoring charge changes.

zhang-ivy · 2022-04-04T13:21:15Z

perses/app/relative_point_mutation_setup.py

+                                                                 topology_proposal.new_topology.residue_topology.name)
+        if charge_diff != 0: # then handle this.
+            new_water_indices_to_ionize = get_water_indices(charge_diff = charge_diff,
+                                                                                           new_positions = new_positions,


Suggested change

new_positions = new_positions,

new_positions = new_positions,

zhang-ivy · 2022-04-04T13:21:30Z

perses/app/relative_point_mutation_setup.py

+        if charge_diff != 0: # then handle this.
+            new_water_indices_to_ionize = get_water_indices(charge_diff = charge_diff,
+                                                                                           new_positions = new_positions,
+                                                                                           new_topology = topology_proposal.new_topology,


Suggested change

new_topology = topology_proposal.new_topology,

new_topology = topology_proposal.new_topology,

zhang-ivy · 2022-04-04T13:21:42Z

perses/app/relative_point_mutation_setup.py

+            new_water_indices_to_ionize = get_water_indices(charge_diff = charge_diff,
+                                                                                           new_positions = new_positions,
+                                                                                           new_topology = topology_proposal.new_topology,
+                                                                                           radius=0.8)


Suggested change

radius=0.8)

radius=0.8)

zhang-ivy · 2022-04-04T13:21:58Z

perses/app/relative_setup.py

+        charge_diff = get_charge_difference(self._ligand_oemol_old, self._ligand_oemol_new)
+        if charge_diff != 0: # then handle this.
+            new_water_indices_to_ionize = get_water_indices(charge_diff = charge_diff,
+                                                                                           new_positions = new_positions,


Suggested change

new_positions = new_positions,

new_positions = new_positions,

zhang-ivy · 2022-04-04T13:22:08Z

perses/app/relative_setup.py

+        if charge_diff != 0: # then handle this.
+            new_water_indices_to_ionize = get_water_indices(charge_diff = charge_diff,
+                                                                                           new_positions = new_positions,
+                                                                                           new_topology = topology_proposal.new_topology,


Suggested change

new_topology = topology_proposal.new_topology,

new_topology = topology_proposal.new_topology,

zhang-ivy · 2022-04-04T13:22:21Z

perses/app/relative_setup.py

+            new_water_indices_to_ionize = get_water_indices(charge_diff = charge_diff,
+                                                                                           new_positions = new_positions,
+                                                                                           new_topology = topology_proposal.new_topology,
+                                                                                           radius=0.8)


Suggested change

radius=0.8)

radius=0.8)

zhang-ivy · 2022-04-04T13:22:52Z

@dominicrufa : Looks good to me! Just added some commit suggestions to improve the formatting.

# Conflicts: # perses/app/relative_point_mutation_setup.py

mikemhenry · 2022-05-02T17:17:50Z

Just 3 tests failing for the same reason:

        # Get the array of water indices (w.r.t. new topology) to turn into ions
>       water_indices = PolymerProposalEngine.get_water_indices(charge_diff = charge_diff_test,
                                                 new_positions = new_pos,
                                                 new_topology = top_proposal._new_topology,
                                                 radius=0.8)
E       AttributeError: type object 'PolymerProposalEngine' has no attribute 'get_water_indices'

perses/tests/test_topology_proposal.py:987: AttributeError

mikemhenry · 2022-05-02T17:19:55Z

Tests just need to get updated, get_water_indices was a static method for PolymerProposalEngine but now is a bare function in charge utilities

ijpulidos · 2022-05-05T17:19:23Z

@mikemhenry these changes should be pretty straightforward to do, I'll be making those if that is ok with you all. Just to get this merged as soon as possible.

mikemhenry · 2022-05-05T17:31:05Z

@ijpulidos go ahead and turn on auto merge for this when you are ready!

codecov · 2022-05-05T17:47:26Z

Codecov Report

Merging #973 (2fa18d6) into main (4901ccb) will increase coverage by 0.06%.
The diff coverage is 93.75%.

add small molecule charge change support with test

2866647

dominicrufa requested a review from zhang-ivy March 21, 2022 19:59

ijpulidos added the priority: high priority high label Mar 21, 2022

creating temporary charge_changing file for the necessary utilities a…

cda4962

…nd refactoring charge changes.

zhang-ivy reviewed Apr 4, 2022

View reviewed changes

zhang-ivy approved these changes Apr 4, 2022

View reviewed changes

ijpulidos self-requested a review April 4, 2022 20:21

ijpulidos added 2 commits April 18, 2022 13:11

Merge branch 'main' into small_mol_charge

bd55391

# Conflicts: # perses/app/relative_point_mutation_setup.py

Merge branch 'main' into small_mol_charge

95c7e94

ijpulidos added this to the 0.10.0 Enhanced CLI with more stable input/output formats milestone May 2, 2022

ijpulidos linked an issue May 2, 2022 that may be closed by this pull request

Support for generalized charge changes #862

Closed

ijpulidos added 2 commits May 5, 2022 13:29

Adapting changes to charge utils new API point.

378f584

Merge branch 'main' into small_mol_charge

2fa18d6

mikemhenry approved these changes May 5, 2022

View reviewed changes

ijpulidos enabled auto-merge (squash) May 5, 2022 17:31

ijpulidos merged commit c9fa0c6 into main May 5, 2022

ijpulidos deleted the small_mol_charge branch May 5, 2022 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add small molecule charge change support with test #973

add small molecule charge change support with test #973

dominicrufa commented Mar 21, 2022

zhang-ivy commented Mar 22, 2022 •

edited

zhang-ivy commented Mar 22, 2022 •

edited

jchodera commented Mar 22, 2022

jchodera commented Apr 1, 2022

zhang-ivy Apr 4, 2022

zhang-ivy Apr 4, 2022

zhang-ivy Apr 4, 2022

zhang-ivy Apr 4, 2022

zhang-ivy Apr 4, 2022

zhang-ivy Apr 4, 2022

zhang-ivy commented Apr 4, 2022

mikemhenry commented May 2, 2022

mikemhenry commented May 2, 2022

ijpulidos commented May 5, 2022

mikemhenry commented May 5, 2022

codecov bot commented May 5, 2022

	new_positions = new_positions,
	new_positions = new_positions,

	new_topology = topology_proposal.new_topology,
	new_topology = topology_proposal.new_topology,

add small molecule charge change support with test #973

add small molecule charge change support with test #973

Conversation

dominicrufa commented Mar 21, 2022

Description

Motivation and context

How has this been tested?

zhang-ivy commented Mar 22, 2022 • edited

zhang-ivy commented Mar 22, 2022 • edited

jchodera commented Mar 22, 2022

jchodera commented Apr 1, 2022

zhang-ivy Apr 4, 2022

Choose a reason for hiding this comment

zhang-ivy Apr 4, 2022

Choose a reason for hiding this comment

zhang-ivy Apr 4, 2022

Choose a reason for hiding this comment

zhang-ivy Apr 4, 2022

Choose a reason for hiding this comment

zhang-ivy Apr 4, 2022

Choose a reason for hiding this comment

zhang-ivy Apr 4, 2022

Choose a reason for hiding this comment

zhang-ivy commented Apr 4, 2022

mikemhenry commented May 2, 2022

mikemhenry commented May 2, 2022

ijpulidos commented May 5, 2022

mikemhenry commented May 5, 2022

codecov bot commented May 5, 2022

Codecov Report

zhang-ivy commented Mar 22, 2022 •

edited

zhang-ivy commented Mar 22, 2022 •

edited