-
Notifications
You must be signed in to change notification settings - Fork 670
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
[WIP] Cif reader #4681
base: develop
Are you sure you want to change the base?
[WIP] Cif reader #4681
Conversation
Hello @hp115! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-08-23 15:11:23 UTC |
Linter Bot Results:Hi @hp115! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
@richardjgowers / @hmacdope would one of you assign the PR to yourself to look after it? You both seem to have an interest in it. |
.. _PDBx: | ||
https://pdb101.rcsb.org/learn/guide-to-understanding-pdb-data/beginner’s-guide-to-pdb-structures-and-the-pdbx-mmcif-format | ||
""" | ||
import gemmi |
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.
for clarity, this is a new MPL-2.0 license dependency we'd be adding.
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.
We would need to make this optional.
|
||
def test_pdbx() | ||
|
||
def test_ |
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.
Shouldn't be too hard to draft tests based on other coordinate readers and some small test files.
xyz = np.zeros((self.natoms, 3), dtype=np.float32) | ||
|
||
for i, (x, y, z) in enumerate(coords): | ||
xyz[i, :] = x, y, z |
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.
A bit unfortunate we need a manual CPython loop over each coordinate. I don't see an obvious alternative from poking around. Gemmi is 90 % C++ it seems, so one might think they could return an appropriate buffer somehow, but that's probably an upstream matter and would likely want to focus on correctness here for now.
Fixes #
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4681.org.readthedocs.build/en/4681/