#1322 Should AtomTypeManipulator.configure() set aromatic information

cdk-1.0.x
accepted
nobody
None
1
2016-08-10
2014-01-13
John May
No

Stephan found this when debugging in the LogP code.

percieveAtomTypesAndConfigureAtoms() will set the aromatic flags based on atom-types. Now we have multiple aromatic models this means information is removed. Configure unset does the job correctly but should atom types and aromaticity be this tightly linked?

Discussion

  • Egon Willighagen

    IMHO, that would be incorrect, because the CDK atom types do not have a concept of aromaticity.

     
  • Egon Willighagen

    No, it should not. There are several alternative models (algorithms) to calculate aromaticity, and the configure() API would need to get additional parameters to indicate which model to use, as well as parameters for that algorithm, like which ring set.

    I suggest closing this report.

     
  • John May

    John May - 2015-01-02

    The bug is that atom typing wipes the information.

     
  • Egon Willighagen

    Ah, simpler... no, that method should only configure information based on atom types, and aromaticity is not an atom type property.

    Do we have a patch, or should I hack up a proposal?

     
  • John May

    John May - 2015-01-02

    I don't know what the correct behaviour is - the current set() and setUnset() is working as expected if aromaticity is tied to atom types. But since this is no longer true (with different aromatic models) aromaticity flags can be removed.

     
  • Egon Willighagen

    Actually, thinking about a patch... atom type definitions can include the idea of aromaticity... but if the type is not explicitly IS_AROMATIC that does not mean it is not aromatic. I'll make a patch for this. That will solve the overwriting.

     
  • Egon Willighagen

    • status: open --> accepted
     
    • John May

      John May - 2016-08-10

      ​That sounds like the correct fix. Thanks!

       

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks