-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for handling PDBs with multiple models #101
Conversation
Hello @a-r-j! Thanks for updating this PR.
Comment last updated at 2022-05-11 23:21:47 UTC |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR, looks great!
@@ -5,23 +5,27 @@ | |||
# License: BSD 3 clause | |||
# Project Website: http://rasbt.github.io/biopandas/ | |||
# Code Repository: https://github.com/rasbt/biopandas | |||
from __future__ import annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. I think type annotations were introduced in Python 3.6. If someone has an older version of Python, they won't be able to run it anyways because of the f-strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I can take care of removing it.
df.df["ANISOU"] = df.df["ANISOU"].loc[df.df["ANISOU"]["model_id"] == model_index] | ||
return df | ||
|
||
def get_models(self, model_indices: List[int]) -> PandasPdb: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thin the from future import annotations is required from this type hint (i didn't want to hassle with defining a typevar - if you remove the import I think this needs to be removed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Let's leave it then
self.df["ANISOU"]["model_id"] = idx_map | ||
return self | ||
|
||
def get_model(self, model_index: int) -> PandasPdb: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Ooh, before you merge it might be worth checking out how to provide a more informative error for structures that only contain a single structure. Eg from biopandas.pdb import PandasPdb
df = PandasPdb().fetch_pdb('3eiy')
a = df.get_model_start_end() returns an empty df and so results in:
|
Good spot! I think they should all be integers and it's an oversight from me |
Eg from biopandas.pdb import PandasPdb
df = PandasPdb().fetch_pdb('3eiy')
a = df.get_model_start_end() returns an empty df and so results in: ... Does |
It doesn't, no, as the labelling function breaks before any selections are made. |
I see. The question, is, do we want this to work? E.g., having always |
Easy fix, it handles single model structures now by mapping all the lines in the PDB file to model_idx 1 |
@rasbt Anything you need from me to get these PRs merged? 😁 |
Ahh, sorry was traveling and forgot to follow-up. Let me check this again properly once I am home tomorrow at my main computer! |
This looked all good to me! Just reformatted biopandas with black to fix the style issues, hence so many changes. Just wanted to do it for the new file but then I thought why not doing it for the whole library. |
Description
Adds a collection of methods to extract and label models from PDB files containing multiple models.
Related issues or pull requests
#50 #100
Pull Request Checklist
./docs/sources/CHANGELOG.md
file (if applicable)./biopandas/*/tests
directories (if applicable)biopandas/docs/sources/
(if applicable)PYTHONPATH='.' pytest ./biopandas -sv
and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g.,PYTHONPATH='.' pytest ./biopandas/classifier/tests/test_stacking_cv_classifier.py -sv
)flake8 ./biopandas