In testAromaticity, why does the loop go from 0 to 5 only? It seems that the first 2 atoms can't be aromatic and there are more than 5 atoms in the molecule.
In testPyrolle (it should be testPyrrole), are you sure that the hydrogen doesn't get included in the first 5 atoms?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I attached two patches, one addressing the issue you raised:
1. first two atoms in the SMILES were not aromatic at all
2. i=0..5 seems copy/paste artifact from first unit test
So, I fixed in my 0002 patch, by shortening the SMILES to only have aromatic atoms (it still fails), and having both unit tests now use a foreach loop.
The 0001 patch I added does some minor JavaDoc updating, adding a @see to another pyrolle unit test, and adding a @cdk.inchi link.
Rajarshi, please review once more, for cdk 1.2.x please. I will work on a fix.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
What branch? Use the category list to specify
In testAromaticity, why does the loop go from 0 to 5 only? It seems that the first 2 atoms can't be aromatic and there are more than 5 atoms in the molecule.
In testPyrolle (it should be testPyrrole), are you sure that the hydrogen doesn't get included in the first 5 atoms?
I think this is the patch for this bug:
https://sourceforge.net/tracker/?func=detail&aid=2976054&group_id=20024&atid=120024
Minor JavaDoc improvements.
Patch to address the 0,5 issue raised by Rajarshi.
Rajarshi,
I attached two patches, one addressing the issue you raised:
1. first two atoms in the SMILES were not aromatic at all
2. i=0..5 seems copy/paste artifact from first unit test
So, I fixed in my 0002 patch, by shortening the SMILES to only have aromatic atoms (it still fails), and having both unit tests now use a foreach loop.
The 0001 patch I added does some minor JavaDoc updating, adding a @see to another pyrolle unit test, and adding a @cdk.inchi link.
Rajarshi, please review once more, for cdk 1.2.x please. I will work on a fix.
Looks good in testPyrolle but still will fail - the loop will go over the H atom which will not be aromatic and so cause a failure
Why would it loop over an implicit hydrogen?
The test fails, I think, simply because there is a bug in the aromaticity detection code... I have a clue about that...
Isn't that H explicit? I thought the parser would keep explicit H's as explicit and not covert to implicit
We are both talking about this SMILES: c1c[nH]cc1
That hydrogen is implicit. In the following variant it would be explicit:
c1cn([H])cc1
D'oh! of course. Should have had coffee instead of tea this morning :(
Applied and pushed.