#785 cdk-legacy

Accepted
closed
nobody
None
master
1
2014-07-22
2014-07-18
John May
No

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

Related

Patches: #785

Discussion

  • Comment on GitHub.

     
    • John May
      John May
      2014-07-18

      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

       
      Last edit: John May 2014-07-18
      • Need a better name that cdkLegacy()?

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

         
        • John May
          John May
          2014-07-19

          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();

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

             
            • John May
              John May
              2014-07-19

              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
                John May
                2014-07-19

                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

                 
  • 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
      John May
      2014-07-19

      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/

       

      Related

      Patches: #785

      Attachments
  • John May
    John May
    2014-07-20

    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?

     
  • Applied and pushed.

     
    • status: open --> closed
    • Group: Needs_Review --> Accepted