Conversation
There was a problem hiding this comment.
Example where gziping the PDB would make this diff hard
for more information, see https://pre-commit.ci
…nto membrane_prototype
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of small things, happy if the [nit] comments don't get addressed / are discarded.
Very close to merge imho.
|
|
||
| class SolvatedPDBComponent(ProteinComponent, BaseSolventComponent): | ||
| """ | ||
| Protein component with explicit solvent and box vectors. |
There was a problem hiding this comment.
| Protein component with explicit solvent and box vectors. | |
| Component with explicit solvent and box vectors. |
Here and everywhere else, given this class is "SolvatedPDBComponent" maybe we shouldn't be saying Protein?
There was a problem hiding this comment.
Changed this in the relevant places.
| * ``box_vectors`` must be an OpenFF quantity with units. | ||
| * Box vectors are serialized and included in equality and hash checks. | ||
| * Construction will fail if box vectors cannot be determined as well as | ||
| if the RDKit molecule has only one disconnected fragment. |
There was a problem hiding this comment.
Am I correct in understanding this is no longer true?
There was a problem hiding this comment.
Yes, removed this here and elsewhere!
| * ``box_vectors`` must be an OpenFF quantity with units. | ||
| * Box vectors are serialized and included in equality and hash checks. | ||
| * Construction will fail if box vectors cannot be determined as well as | ||
| if the RDKit molecule has only one disconnected fragment. |
There was a problem hiding this comment.
Am I correct in understanding this is no longer true?
| Notes | ||
| ----- | ||
| * ``box_vectors`` must be an OpenFF quantity with units. | ||
| * Box vectors are serialized and included in equality and hash checks. |
There was a problem hiding this comment.
[nit] This bit of the docstring is user facing, it's likely going to be a bit confusing to have this - I would remove this bullet point.
There was a problem hiding this comment.
Removed this!
| If ``box_vectors`` is not an OpenFF Quantity. | ||
| ValueError | ||
| If ``box_vectors`` are not valid box vectors. | ||
| If the RDKit molecule contains only one disconnected fragment. |
There was a problem hiding this comment.
See above, is this still being done?
There was a problem hiding this comment.
Removed this.
| if not _box_vectors_are_in_reduced_form(box): | ||
| raise ValueError(f"box_vectors: {box} are not in OpenMM reduced form") | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
[nit] cc @atravitz - given we don't really use these "water" methods in this class directly, thoughts on moving this to the membrane specific one?
There was a problem hiding this comment.
I'm in favor of moving it to the membrane-specific one! Plus, it's easier to upstream something (non-api-breaking) than to make it more specific later (api-breaking)
There was a problem hiding this comment.
Moved the water counting methods to the SolvatedPDBComponent.
| <Atom name="H73" alt1="3H5M"/> | ||
| </Residue> | ||
| <Residue name="HOH" alt1="H20" alt2="WAT" alt3="SOL" alt4="TIP3" alt5="TP3"> | ||
| <Residue name="HOH" alt1="H20" alt2="WAT" alt3="SOL" alt4="TIP3" alt5="TP3" alt6="T4P" alt7="SPC"> |
There was a problem hiding this comment.
Can you add an issue about this - we should try to get upstream to update this
There was a problem hiding this comment.
Raised an issue here: openmm/openmm#5212
atravitz
left a comment
atravitz
left a comment
There was a problem hiding this comment.
Looks good to me, just some non-blockin nits. Including that many of these tests are quite long (3-45 seconds, whereas most gufe unit tests are <1 second), which we've (so far) been able to avoid in gufe. If we want to get this PR merged though, I can work on speeding up the tests in a follow-up PR.
| if not _box_vectors_are_in_reduced_form(box): | ||
| raise ValueError(f"box_vectors: {box} are not in OpenMM reduced form") | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
I'm in favor of moving it to the membrane-specific one! Plus, it's easier to upstream something (non-api-breaking) than to make it more specific later (api-breaking)
| # 3. Infer box vectors if requested | ||
| if infer_box_vectors: | ||
| box = cls._estimate_box(structure, padding=box_padding) | ||
| warnings.warn( |
There was a problem hiding this comment.
@dotsdl - logging question: since this a class method and there is no tokenizable instantiated yet - is this the preferred way to warn?
There was a problem hiding this comment.
I think this makes the most sense, yes. As you said, there is no logger for the GufeTokenizable since it hasn't been instantiated yet, so our best bet is to use warnings here.
atravitz
left a comment
atravitz
left a comment
There was a problem hiding this comment.
added two change suggestions that should fix docs @hannahbaumann!
Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>
Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
| ... | ||
|
|
||
| def validate(self, **kwargs): | ||
| def validate(self): |
There was a problem hiding this comment.
Removed the **kwargs to make mypy happy, alternatively I could also add the **kwargs in the validate in the child class, but I'm not sure if it's necessary?
|
No API break detected ✅ |
* Add an ExplicitPDBComponent and a ProteinMembraneComponent * Fix the residue names for PDB reading to also include T4P and SPC --------- Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com> Co-authored-by: Josh Horton <Josh.Horton@newcastle.ac.uk> Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
6 participants
This PR addresses following issues:
Tips
Since this will create a commit, it is best to make this comment when you are finished with your work.
Checklist
newsentryDevelopers certificate of origin