Tests for capped AAs #92
Conversation
|
Hello @ijpulidos! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
|
tests are expected to fail here, that should be OK, since we would be implementing the protocol to pass the tests in future PRs. |
feflow/protocols/protein_mutation.py
Outdated
| @@ -2,14 +2,51 @@ | |||
| Implementation of protein mutation protocol based on Nonequilibrium Cycling, using OpenMM as | |||
| MD engine. | |||
| """ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
Are we really going to have a different setupunit / protocol for this? I thought the aim was to just make sure our current Protocols could just handle this use case?
There was a problem hiding this comment.
Yes probably, for now I create a basic skeleton for it, but we would probably use the same SetupUnit for all the protocols as possible. I can remove this placeholder if that makes it better.
feflow/protocols/protein_mutation.py
Outdated
| from typing import Dict, Any | ||
|
|
||
| from gufe import ChemicalSystem, Context, ProtocolUnit, AtomMapping | ||
|
|
||
|
|
||
| def mutate_with_pdbfixer(filename: str | Path, mutation_spec: str, chain_id: str = "A", output_pdb=None): |
There was a problem hiding this comment.
I would suggest putting this in a separate place, like a feflow.setup or feflow.utils.
Reasoning: this is something you'll do before you reach the Protocol, so from a user standpoint, it's a setup tools (indeed, it might even make sense to make a separate toolling akin to kartograf or konnektor if this goes beyond just a single method!)
There was a problem hiding this comment.
Yes, 100%, this was something I added temporarily and forgot to remove. My bad!
There was a problem hiding this comment.
It shouldn't be here at all.
feflow/protocols/protein_mutation.py
Outdated
| filename | ||
| mutation_spec | ||
| chain_id | ||
| output_pdb |
feflow/tests/data/ALA_capped.pdb
Outdated
There was a problem hiding this comment.
Could you move these under a sub-category of the data folder?
It's maybe a little bit of eager tidying up, but annotating the data by folders would be super useful!
There was a problem hiding this comment.
Yes, that's fine with me. I did think about that.
|
It seems like the only tests failing here are the ones expected to be failing https://github.com/OpenFreeEnergy/feflow/actions/runs/11185794043/job/31099415246?pr=92#step:8:298 this one should be good to go now. We will be implementing the code for the tests to pass in different PRs. This is to make reviews more modular and easier to do. |
These set of changes implement basic tests for capped amino acids.
Solves #78
It follows the same testing structure that was created for the
NonEquilibriumCyclingProtocol, which is based on fixtures.To avoid having to generate the mapping every time, the mappings are being deserialized from
jsonfiles.Please note that this is aimed to be merged to a feature branch where we would have the complete functional MVP for the protein mutation protocol.