#362 SMSD patch

Needs_Revision
closed
Asad
master (162)
5
2012-10-08
2011-06-20
Asad
No

SMSD patch with latest code
a) Isomorphism
b) Substructure search
c) Thread safe

Discussion

  • Asad, this is a massive patch. Just in the first 4% I see numerous problems. Have you run the CDK QA system against it? Please do. I know this is painful work, but we all have to do it.

    The current patch has way to much I would comment on, for me to review this patch in the next weeks. Solving all OpenJavaDocCheck, PMD, JUnit (coverage) problem will very much help.

     
  • Asad
    Asad
    2011-06-21

    Hi Egon,
    How to run CDK QA system ?

    Asad

     
  • ant -Dmodule=smsd qa-module

     
  • Asad
    Asad
    2011-07-26

    Cleaner patches as advised

     
    Attachments
  • Asad
    Asad
    2011-07-26

    Status update of the patches after running the QA.

    ant -Dmodule=smsd qa-module
    Buildfile: /users/Asad/Software/GITROOT/cdksmsdgithub/build.xml

    checkPlatforms:

    check:

    dist.init:

    qa-module:
    [echo] Running QA for CDK's smsd module

    checkPlatforms:

    check:

    noJunit:

    test-module:
    [echo] Testing classes for CDK's smsd module.
    [echo] CDK dependencies defined: true
    [echo] Library dependencies defined: true
    [echo] Developer Library dependencies defined: true
    [junit] Running org.openscience.cdk.modulesuites.MsmsdTests
    [junit] Tests run: 222, Failures: 2, Errors: 0, Time elapsed: 8.498 sec
    [junit] Test org.openscience.cdk.modulesuites.MsmsdTests FAILED

    ojdcheck-module:
    [mkdir] Created dir: /users/Asad/Software/GITROOT/cdksmsdgithub/reports/ojdcheck
    [javadoc] Generating Javadoc
    [javadoc] Javadoc execution
    [javadoc] Loading source files for package org.openscience.cdk.normalize...
    [javadoc] Loading source files for package org.openscience.cdk.smsd...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.matchers...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.mcgregor...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.mcsplus...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.rgraph...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.single...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.vflib...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.vflib.builder...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.vflib.interfaces...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.vflib.map...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.vflib.query...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.algorithm.vflib.substructure...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.filters...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.global...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.helper...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.interfaces...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.ring...
    [javadoc] Loading source files for package org.openscience.cdk.smsd.tools...
    [javadoc] Constructing Javadoc information...
    [javadoc] Could not load class 'net.sf.cdk.tools.checkdoctest.MissingGithashTagletTest': net.sf.cdk.tools.checkdoctest.MissingGithashTagletTest
    [javadoc] Could not load class 'net.sf.cdk.tools.checkdoctest.MissingModuleTagletTest': net.sf.cdk.tools.checkdoctest.MissingModuleTagletTest
    [javadoc] Could not load class 'net.sf.cdk.tools.checkdoctest.IncorrectBugNumberTagletTest': net.sf.cdk.tools.checkdoctest.IncorrectBugNumberTagletTest

    test-module:
    [echo] Running PMD checks for CDK's smsd module.
    [mkdir] Created dir: /users/Asad/Software/GITROOT/cdksmsdgithub/reports/pmd

    test-module:
    [echo] Checking for unused code in CDK's smsd module.
    [mkdir] Created dir: /users/Asad/Software/GITROOT/cdksmsdgithub/reports/pmd-unused

    test-module:
    [echo] Checking for migrating code updates in CDK's smsd module.
    [mkdir] Created dir: /users/Asad/Software/GITROOT/cdksmsdgithub/reports/pmd-migrating

    BUILD SUCCESSFUL
    Total time: 33 seconds

     
  • Asad,

    it took some effort to rebase it on the current master branch, and the patch is almost 1MB large... I had a look at it, and have many requests. It's far from the quality of much of the core CDK code, and I honestly have no clue how to judge this patch. Code review does not work of patches of this size.

    I have attached my set of patches resulting from the rebase I did, and like you to verify if that looks good and nothing went wrong.

    I ran the 'qa-module' too and it finds quite a few JavaDoc and PMD problems. Please check the cdk/report/ folder for all output from this qa-module target.

    Asad, if I suggest to accept this patch for master, I will file a lot of bug reports, for all sorts of things. Will you promptly address those, with one patch for one bug report, and sending those in one by one, and not as another massive patch? I will file bug reports for JavaDoc and PMD issues, but also plan to file 'bug' reports for classes which seem to have been duplicated, like CDKRGraph; I see no need why it cannot use the CDK version... also, a class like HanserRingFinger seems it can move to another CDK package...

    Rajarshi, Christoph, are you happy with such a set up?

    Asad, additionally, please remove the output to STDOUT in the main source and in the test classes.

     
  • Rebased on today's master.

     
    Attachments
  • Reluctantly, this patch should really be rejected in favour of making smaller, separate patches on topics (like a), b), c)).

    I will use Egon's patches to guide this process, and I see that some of them are javadoc fixes, so hopefully those will apply directly in the topic branch.