Skip to content
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

Merged
merged 16 commits into from Oct 28, 2016
Merged

Improved API for atom creation. #246

merged 16 commits into from Oct 28, 2016

Conversation

johnmay
Copy link
Member

@johnmay johnmay commented Oct 16, 2016

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.

new Atom(6, 3); // Me direct
new Atom("CH3"); // Me verbose

Copy link
Member

@egonw egonw left a 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() {
Copy link
Member

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...

@johnmay
Copy link
Member Author

johnmay commented Oct 16, 2016

All constructor tests need to be there and not in the abstract one. You can check, also didn't implement the DebugAtom one.

@johnmay
Copy link
Member Author

johnmay commented Oct 16, 2016

Need to fix some test regressions but should be no problem.

@johnmay
Copy link
Member Author

johnmay commented Oct 16, 2016

Hmm, this broke a lot. Tones of usages in PDB code parsing in 'C1' as the symbol and not the ID!

@egonw
Copy link
Member

egonw commented Oct 16, 2016

"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...

@johnmay
Copy link
Member Author

johnmay commented Oct 16, 2016

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.

@johnmay
Copy link
Member Author

johnmay commented Oct 19, 2016

Okay will wait for travis to finish but I think this can now be merged.

@johnmay
Copy link
Member Author

johnmay commented Oct 19, 2016

Hmmm travis tests are disabled... odd. Build is still broken for me, two secs.

@johnmay
Copy link
Member Author

johnmay commented Oct 19, 2016

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).

@johnmay
Copy link
Member Author

johnmay commented Oct 19, 2016

1CKV:

CDK test

ATOM      1  N   MET     1      52.189 -29.640  -8.047  1.00  0.00           N  
ATOM      2  CA  MET     1      52.849 -28.544  -7.289  1.00  0.00           C  
ATOM      3  C   MET     1      52.757 -27.220  -8.041  1.00  0.00           C  
ATOM      4  O   MET     1      52.091 -26.286  -7.595  1.00  0.00           O  
ATOM      5  CB  MET     1      54.314 -28.923  -7.063  1.00  0.00           C  
ATOM      6  CG  MET     1      54.542 -29.755  -5.812  1.00  0.00           C  
ATOM      7  SD  MET     1      55.942 -30.882  -5.974  1.00  0.00           S  
ATOM      8  CE  MET     1      57.188 -29.779  -6.637  1.00  0.00           C  
ATOM      9 1H   MET     1      51.238 -29.311  -8.314  1.00  0.00           H  
ATOM     10 2H   MET     1      52.768 -29.838  -8.887  1.00  0.00           H  
ATOM     11 3H   MET     1      52.136 -30.470  -7.423  1.00  0.00           H  
ATOM     12  HA  MET     1      52.357 -28.442  -6.333  1.00  0.00           H  

RSDB

ATOM      1  N   MET A   1      52.189 -29.640  -8.047  1.00  0.00           N  
ATOM      2  CA  MET A   1      52.849 -28.544  -7.289  1.00  0.00           C  
ATOM      3  C   MET A   1      52.757 -27.220  -8.041  1.00  0.00           C  
ATOM      4  O   MET A   1      52.091 -26.286  -7.595  1.00  0.00           O  
ATOM      5  CB  MET A   1      54.314 -28.923  -7.063  1.00  0.00           C  
ATOM      6  CG  MET A   1      54.542 -29.755  -5.812  1.00  0.00           C  
ATOM      7  SD  MET A   1      55.942 -30.882  -5.974  1.00  0.00           S  
ATOM      8  CE  MET A   1      57.188 -29.779  -6.637  1.00  0.00           C  
ATOM      9  H1  MET A   1      51.238 -29.311  -8.314  1.00  0.00           H  
ATOM     10  H2  MET A   1      52.768 -29.838  -8.887  1.00  0.00           H  
ATOM     11  H3  MET A   1      52.136 -30.470  -7.423  1.00  0.00           H  
ATOM     12  HA  MET A   1      52.357 -28.442  -6.333  1.00  0.00           H  
ATOM     13  HB2 MET A   1      54.663 -29.488  -7.914  1.00  0.00           H  
ATOM     14  HB3 MET A   1      54.898 -28.019  -6.979  1.00  0.00           H  
ATOM     15  HG2 MET A   1      54.729 -29.089  -4.983  1.00  0.00           H  
ATOM     16  HG3 MET A   1      53.653 -30.334  -5.614  1.00  0.00           H  
ATOM     17  HE1 MET A   1      56.833 -29.349  -7.561  1.00  0.00           H 

@johnmay
Copy link
Member Author

johnmay commented Oct 19, 2016

Possible to handle.. but need keep a hybrid approach.

@johnmay
Copy link
Member Author

johnmay commented Oct 19, 2016

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.

@egonw egonw merged commit ddbc988 into master Oct 28, 2016
@johnmay johnmay deleted the patch/atom-creation branch February 12, 2017 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants