Update solventcomponent to encompass all composition properties #262
Conversation
|
Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-12-27 23:52:45 UTC |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 99.20% 99.23% +0.02%
==========================================
Files 36 36
Lines 1898 1963 +65
==========================================
+ Hits 1883 1948 +65
Misses 15 15 ☔ View full report in Codecov by Sentry. |
| @@ -535,7 +537,7 @@ def to_keyed_dict(self, include_defaults=True) -> Dict: | |||
|
|
|||
| if not include_defaults: | |||
| for key, value in self.defaults().items(): | |||
| if dct.get(key) == value: | |||
| if np.all(dct.get(key) == value): | |||
There was a problem hiding this comment.
This is probably the wrong way to do this
| Defined the shape of the solvent box being built. Can be one of | ||
| 'cube', 'dodecahedron', and 'octahedron'. Cannot be defined alongside | ||
| `box_vectors`. Default 'cube' | ||
| box_vectors : openff.units.unit.Quantity, optional |
There was a problem hiding this comment.
I'm not sure this is fitting the model we've got currently. ChemicalSystem is meant to capture things that can affect the free energy differences between different ChemicalSystems. This instead seems to be trying to capture a molecular simulation setup, which is different. The solvation additions here shouldn't be affecting the measured (D)DG; padding should be enough that isn't affecting (D)DG, box shape should also be unimportant.
I think the fields num_solvent / padding / vectors are implementation details and should be in a Settings object not here. E.g. if I ran ChemicalSystem(w/ LigA) -> ChemicalSystem(w/ LigB) using a triclinic box, and ChemicalSystem(w/ LigA) -> ChemicalSystem(w/ LigB) with a rectangular box, these two ChemicalSystem(w/ LigA) are identical.
| box_shape: Optional[Literal['cube', 'dodecahedron', 'octahedron']] = 'cube', | ||
| box_vectors: Optional[unit.Quantity] = None,): |
There was a problem hiding this comment.
Once we get to storing boxes, I'm not sure it makes sense to store the box_shape independently of the vectors, the vectors can/should define the shape.
There was a problem hiding this comment.
Not everyone can handle dealing with both - it's too much to ask users to provide box vectors a priori.
| ion_concentration: unit.Quantity = 0.15 * unit.molar): | ||
| ion_concentration: Optional[unit.Quantity] = 0.15 * unit.molar, | ||
| solvent_padding: Optional[unit.Quantity] = 1.2 * unit.nanometer, | ||
| solvent_density: Optional[unit.Quantity] = None, |
There was a problem hiding this comment.
this is probably worth including, as it does affect the reduced potential
There was a problem hiding this comment.
I'm not sure what you mean here - density is a function of N and volume, everything here defines both of those things.
| Defined the shape of the solvent box being built. Can be one of | ||
| 'cube', 'dodecahedron', and 'octahedron'. Cannot be defined alongside | ||
| `box_vectors`. Default 'cube' | ||
| box_vectors : openff.units.unit.Quantity, optional |
There was a problem hiding this comment.
Putting in box_vectors will be needed for explicit solvents, but I don't think we'll need to backport that field to this definition
|
@richardjgowers solvation, particularly the number of waters does affect both the dH (which we will want to be able to calculate), and the dG under certain circumstances. |
Fixes #261