Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
- Coverage 98.35% 97.31% -1.04%
==========================================
Files 10 11 +1
Lines 487 597 +110
==========================================
+ Hits 479 581 +102
- Misses 8 16 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| import prolif as plf | ||
|
|
||
|
|
||
| class ProLIFAnalysis: |
There was a problem hiding this comment.
Can you subclass AnalysisBase here?
There was a problem hiding this comment.
Yup should be doable, will look into it :)
Wanted to wait and see how RMSD will look like to use an identical structure and have this initial PR to have bulletpoints to discuss tomorrow during the meeting and also have an easier overview on what was changed etc. :)
There was a problem hiding this comment.
We leave it as is for now and will change this when Prolif implements per frame analysis.
|
I am not sure if this should be handled in the analysis class or in a protocol before running this class but there is an issue when doing this kind of analysis on trajectories from our hybrid topology protocol, where the connectivity of the end state ligands and identities of the atoms can get confused see this example on end stateB when looking at torsions. Should we add an option to provide and rdkit molecule for the ligand and use this to correct the ligand atom group automatically or should we have users fix this beforehand? This will be needed in the torsion analysis as well so a general function which could fix an atom group might be a good idea? |
…FreeEnergy/openfe_analysis into prolif_class_implementation
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @talagayev , this looks good! The main part I'm not sure about is the run function, maybe we can discuss that tomorrow!
…FreeEnergy/openfe_analysis into prolif_class_implementation
|
@hannahbaumann Code would be ready for another round of review :) |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @talagayev , this looks great! i finally got around to a full review, please let me know if you have any questions!
| water_order: int = 3, | ||
| protein_cutoff: float = 12.0, | ||
| water_cutoff: float = 8.0, | ||
| interactions: Optional[Sequence[str] | str] = None, |
There was a problem hiding this comment.
| interactions: Optional[Sequence[str] | str] = None, | |
| interactions: Sequence[str] | Literal["all"] | None, |
| RDKit/ProLIF can detect donors/acceptors and bonded hydrogens. | ||
| """ | ||
| # Protein: guess on the full protein so any pocket residue later has bonds | ||
| guess_ligand_bonds(self.universe.select_atoms("protein")) |
There was a problem hiding this comment.
Would it make sense to rename the function in utils to just guess_bonds or so? since the ligand part may be confusing for the protein and water.
| else: | ||
| self.times = None | ||
| except Exception: | ||
| self.frames = None |
There was a problem hiding this comment.
Do we really need the try/except here? What happens if this was removed?
| return self | ||
|
|
||
| @property | ||
| def ifp(self): |
There was a problem hiding this comment.
Maybe add type hint here of what the return would be.
| analysis = ProLIFAnalysis(u, ligand_ag, interactions=["WaterBridge"]) | ||
|
|
||
| assert analysis._parameters is not None | ||
| assert "WaterBridge" in analysis._parameters |
There was a problem hiding this comment.
Would it make sense to also test that there is a "WaterBridge" column in the interaction output?
| calls = {} | ||
| traj = {0: None} | ||
|
|
||
| def ag(n): |
There was a problem hiding this comment.
Maybe you could also move some of these things that are repeated across different tests into fixtures?
| from openfe_analysis.prolif import ProLIFAnalysis | ||
| from openfe_analysis.reader import FEReader | ||
|
|
||
|
|
There was a problem hiding this comment.
These tests look great! I think what might be missing is a testing of the start/stop/step in run, and to check the respective times, frames, n_frames when you slice the data like that.
Resolved the test_prolif.py conflict by keeping the refactored version (universe/ligand_ag fixtures, WaterBridge test renamed to *_warns_and_raises, with-water test removed, plotting tests moved to tests/utils/test_plotting.py). prolif.py, plotting.py and conft.py (Zenodo fix #125) auto-merged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…FreeEnergy/openfe_analysis into prolif_class_implementation
First draft of the ProLIF implementation into OpenFE Analysis.
Changes made:
ProLIFAnalysisclass for calculation of interactions using ProLIFtest_prolif.py, which mainly look into if interactions were calculated sucessfully and protein/ligand conversion fromMDAnalysisintoProLIFworked sucessfully.Currently that is the first draft and PR is to see that the tests work etc.
Some notes:
pd.DataFrameformat, since that is easy to see if it worked. We can see what the best way is to present the results, be it 2D visualization of the interactions, dataframes or some other format.MDAnalysisis sometimes tricky, which is the case here. So the code currently needs to guess bonds, which it does here:universe.select_atoms("protein").guess_bonds(vdwradii=vdwradii)But for the
vdwradiiit wants some values forCl,Bretc. that we need to provide, so we need to see what defaults we select there.3, which means it can be something like:Ligand--Water--Water--Water--ProteinProLIFAnalysis.run()there we can also look into what the defaults have to be, how many cores and if the interactions should beatombased (Ligand with Protein atom) orresiduebased (Ligand with Protein residue).This is the first draft, will be updated during further development :)
PS. A small thing I also noticed, since I tested it on different PCs with one being with Windows currently somehow
python 3.14has module loading Issues withrdkitand thus I had to downgrade there topython 3.13.