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
Improved API for atom creation. #246
Conversation
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.
Another question would be: have the builder(s) been updated?
@@ -72,6 +72,91 @@ public void testAtom_String() { | |||
} | |||
|
|||
@Test | |||
public void testAtom_NH4plus_direct() { |
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 guess this method is now in two FooAtomTest classes? Have you tried placing it in AbstractAtomTest? That was created to have tests that should be run for all IAtom implementations...
All constructor tests need to be there and not in the abstract one. You can check, also didn't implement the DebugAtom one. |
Need to fix some test regressions but should be no problem. |
Hmm, this broke a lot. Tones of usages in PDB code parsing in 'C1' as the symbol and not the ID! |
"PDB code parsing in 'C1' as the symbol" ... that is actually probably also not an ID (or not global, anyway), but it's what I would call an atom type ID... and, yes, I think the PDB code should be fixed, not your code... |
…, the pdb_atomtypes.xml shows the correct config.
Okay lots to fix.. but this is moving in the right direction. Only failure I have is the PDBReader which doesn't know about nucleotide atom types. |
Okay will wait for travis to finish but I think this can now be merged. |
Hmmm travis tests are disabled... odd. Build is still broken for me, two secs. |
Looking deeper, some of the test PDB files don't match the RCSB ones.. eg. 1XKQ has an atom 'AP' (???)... whilst if I go and download it again it's 'PA' (Phos). |
1CKV: CDK test
RSDB
|
Possible to handle.. but need keep a hybrid approach. |
Okay all good now, tests parsing without any changes to PDB files. I missed the atom symbol from the end of the line, previous impl took this then through it away with the PDB Atom Type matcher. |
I've wanted the element/hydrogen count int one for a while. Writing up some documentation recently I realised extending the string one would make things much easier for beginners, and is quite efficient so can be used in tests.