#785 cdk-legacy

[John May]

This patch creates the top level module 'cdk-legacy' where we can place deprecated classes, and tools which are not maintained or considered harmful. Fairly large due to unforeseen regressions but should be no additional test failures. This first patch only moves over the aromaticity perception and SwissArmyKnife.

https://github.com/johnmay/cdk/compare/cdk:master...patch/deprecated-module

[Egon Willighagen]

Comment on GitHub.

[John May]

Thanks,

Have commented there, but in summary

1. Need a better name that cdkLegacy()? cdkVersionOnePointFour() ? The legacy was to discourage its (the utility method) use in favour of the normal constructor.
2. I’ll fix the copyright statements (done - was only the CDKHARD)
3. I’ll add a unit test that ensure atomic number remains untouched (done).
4. “X” is less damaging than IAtomType.UNSET at the moment.
5. Not sure on AtomTypeManipulator? I understand the logic - “configure this atom” - but if I have values set I don’t want them to be corrupted….

J

[Egon Willighagen]

Need a better name that cdkLegacy()?

So, how is this model different from cdk() ?

[John May]

This is purely a convenience method… note they are not the same object. The Aromaticity.cdkLegacy() combined the ‘cdk()’ model with a set of cycles.

ElectronDontation ed = ElectronDontation.cdk();
Aromaticity arom = new Aromaticity(ed, Cycles.cdkAromaticSet());
Aromaticity arom = Aromaticity.cdkLegacy();

[Egon Willighagen]

then I would prefer it to be called cdk() or cdkModel()...

[John May]

I would prefer to use that for one that doesn’t only test a subset of rings:

new Aromaticity(cdk(), or(all(), edgeShort()));

This cdkLegacy() is literally to replicate the existing method that only checks a subset of rings so that I could relatively harmlessly replace the usage in tests.

J

[John May]

Just to clarify some more… the new arom is intended to be inject into a tool.

FP for = new Fingerprinter(new Aromaticity(...));

The ‘Aromaticity.cdkLegacy()’ is for the existing usages that don’t do this.

J

[Egon Willighagen]

OK, so the model is the same, except that the legacy CDKHueckelAromTest class was using SSSR, which is the subset you are referring to?

If so, then I agree.

[John May]

Yep - we could change the tests over to the new all cycles based method but the patch was already large.

J

Sent from my iPhone

On 19 Jul 2014, at 10:30, "Egon Willighagen" egonw@users.sf.net wrote:

OK, so the model is the same, except that the legacy CDKHueckelAromTest class was using SSSR, which is the subset you are referring to?

If so, then I agree.

[patches:#785] cdk-legacy

Status: open
Group: Needs_Review
Created: Fri Jul 18, 2014 02:06 PM UTC by John May
Last Updated: Fri Jul 18, 2014 05:36 PM UTC
Owner: nobody

This patch creates the top level module 'cdk-legacy' where we can place deprecated classes, and tools which are not maintained or considered harmful. Fairly large due to unforeseen regressions but should be no additional test failures. This first patch only moves over the aromaticity perception and SwissArmyKnife.

https://github.com/johnmay/cdk/compare/cdk:master...patch/deprecated-module

Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/cdk/patches/785/

To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/

[Egon Willighagen]

John, maybe we should sit down with Nina and make a page like this, with the various new Aromaticity() options: http://apps.ideaconsult.net:8080/ambit2/depict/cdk?search=c1ccc2c%28c1%29CC4NCCc3cccc2c34

[John May]

Possible, but I'm not sure how useful it would be?

[John May]

I've added some additional documentation to the 'cdkLegacy()' method that gives more details that just 'replicates CDKHAD'.

What else is needed on this patch?

[Egon Willighagen]

Applied and pushed.