OpenBabelParser and Tests #12
Conversation
|
Hello @lunamorrow! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-07-31 05:52:44 UTC |
| # # PDB only | ||
| # for vals, Attr, dtype in ( | ||
| # (altlocs, AltLocs, object), | ||
| # (chainids, ChainIDs, object), | ||
| # (occupancies, Occupancies, np.float32), | ||
| # (tempfactors, Tempfactors, np.float32), | ||
| # ): | ||
| # if vals: | ||
| # attrs.append(Attr(np.array(vals, dtype=dtype))) |
There was a problem hiding this comment.
How do you plan on handling this?
| # assert_equal(expected, top.elements.values) | ||
|
|
||
| # need to check other attrs including: | ||
| # 'ids', 'names', 'resids', 'resnums', 'chiralities', 'segids', 'charges' |
There was a problem hiding this comment.
I would add at least three more OBmols originating from files to test. Try and vary the chemistry, something large and protein-like perhaps, an SDF with a complex organic ligand, and something rather metallic. You can set these up in conftest.py
|
@lunamorrow you should also try and fix #7 (in a separate PR ) so that we can verify that the tests are working as intended here. |
There was a problem hiding this comment.
Nice improvements Luna!
My main concern is about the use of OB v2 instead of v3 since v2 is quite old now (and v3 isn't exactly brand new). I'm happy with sticking to v2 for now, getting everything working, and then make the few required changes to switch to v3 in another PR if it's easier.
Also sorry for the lack of more regular comments resulting in me posting too many suggestions in one go 😅
|
|
||
| # convert atomic number to element | ||
| elements.append(OBElementTable().GetSymbol(atom.GetAtomicNum())) |
There was a problem hiding this comment.
nitpick: since we need to reuse that element table, better to assign an instance as a constant at the top of the module and reuse that same instance here
Adding changes made in PR #12
… basic ones (atoms, bonds, segments and residues)
lunamorrow
force-pushed
the
parser_and_tests
branch
from
9bc9b74 to
0d4962c
Compare
Adding changes made in PR #12
In reference to #10. This is a draft of the code with the initial functionality starting to be roughly scaffolded out for feedback.
PR Checklist