search

MDAnalysis / mda-openbabel-converter

MIT License
0 stars 1 forks source link

OpenBabelReader and Tests #16

Closed lunamorrow closed 3 weeks ago

lunamorrow commented 2 months ago

In reference to #5. This is a draft of the code with the initial functionality starting to be roughly scaffolded out for feedback.

PR Checklist

pep8speaks commented 2 months ago

Hello @lunamorrow! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 52:13: E128 continuation line under-indented for visual indent Line 53:13: E128 continuation line under-indented for visual indent Line 59:80: E501 line too long (81 > 79 characters)

Line 17:61: W292 no newline at end of file

Line 19:1: E302 expected 2 blank lines, found 1 Line 35:13: E128 continuation line under-indented for visual indent Line 36:13: E128 continuation line under-indented for visual indent

Comment last updated at 2024-08-06 01:37:45 UTC
exs-cbouy commented 2 months ago

Nice work Luna! cat-thumbs-up

xhgchen commented 2 months ago

Great work! Loving the progress :)

lunamorrow commented 2 months ago

Could you also add a test for mols without any coordinates making sure that they are all nan?

@exs-cbouy It looks like null position in OpenBabel is stored as a (0, 0, 0) vector. I don't want to report that as a position when its not, so I'm thinking that I can check that if the whole molecules position vectors are (0, 0, 0) that there is no position info and to set it to nan (or not even set it) and issue a warning. That will prevent molecules with an atom at a valid (0, 0, 0) position from being flagged. Thoughts?

Edit: I've got it, just had to change my conditions for empty coordinates. Pushing changes now.