#1322 Should AtomTypeManipulator.configure() set aromatic information

[John May]

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?

[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]

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]

To be specific - https://github.com/cdk/cdk/blob/master/base/standard/src/main/java/org/openscience/cdk/tools/manipulator/AtomTypeManipulator.java#L89

[John May]

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]

Patch: https://github.com/cdk/cdk/pull/220

[John May]

​That sounds like the correct fix. Thanks!