[Cdk-devel] [ cdk-Patches-2976058 ] Tests for aromatic smiles

[SourceForge.net <no...@so...>]

Patches item #2976058, was opened at 2010-03-24 14:18
Message generated for change (Comment added) made by rajarshi
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2976058&group_id=20024

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: cdk-1.2.x
>Group: Accepted
>Status: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: Stefan Kuhn (shk3)
Assigned to: Rajarshi Guha (rajarshi)
Summary: Tests for aromatic smiles

Initial Comment:
Added two tests for aromatic smiles. One works, the other doesn't. Unclear why for me.

----------------------------------------------------------------------

>Comment By: Rajarshi Guha (rajarshi)
Date: 2010-03-27 13:45

Message:
D'oh! of course. Should have had coffee instead of tea this morning :(

Applied and pushed.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-03-27 13:31

Message:
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

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2010-03-27 13:08

Message:
Isn't that H explicit? I thought the parser would keep explicit H's as
explicit and not covert to implicit

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-03-27 13:03

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

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2010-03-27 12:45

Message:
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

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-03-27 06:49

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

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-03-27 06:25

Message:
I think this is the patch for this bug:

https://sourceforge.net/tracker/?func=detail&aid=2976054&group_id=20024&atid=120024

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2010-03-25 20:28

Message:
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?

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2010-03-25 01:10

Message:
What branch? Use the category list to specify

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2976058&group_id=20024