Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#120 Signatures package and module

Needs_Revision
closed
None
5
2012-10-08
2009-10-19
No

The classes relating to signatures are in the first of these patches, and tests are in the second two.

Discussion

  • Rajarshi Guha
    Rajarshi Guha
    2009-10-20

    Patches look good. A few comments

    • I don't see any code implementing the signatures themselves. Is that in a separate patch?
    • In 0003, if the tests are working OK, I'd remove the calls to System.out.println (even though they are commented out)

    However, I can't apply 0001 (and so didn't try the others):

    error: patch failed: build.xml:994
    error: build.xml: patch does not apply

    I'm assuming these patches were meant to be applied to master (since they implement new parts of the API)?

     
  • 1) 0001-New-signature-package.patch is the one with the actual signature code in!
    2) I can remove the calls to system.out
    3) Yes, they are meant to be applied to master, but I don't know why the one you tried failed :(
    4) I hate this patches system...

     
  • Gilleain, I have not had time to look at the patches yet... but please do not worry about the build.xml stuff...

    Rajarshi, please remove the build.xml bits from the patch (with a plain text editor), and I will take care of adding the module to the build system instead...

     
  • Rajarshi Guha
    Rajarshi Guha
    2009-10-22

    Aah OK. Yes, after getting rid of build.xml parts, it all applies cleanly. Patch looks good and tests are all OK. A few comments

    • shouldn't TestSignature extend some abstract test class (CDKTest?)
    • There is a lack of Javadocs in a number of source files (OrbitElement, Signature, TargetAtomicSignature).
    • Finally, I'm assuming that the user visible portion of this package would be Signature (and TargetAtomoicSignature?). Would it be possible to make the helper classes (Edge, Node etc) package protected, so that they wouldn't be visible in the public API?

    Otherwise good to go

     
  • Right, so patch 0001-Improved-javadocs-in-the-signature-package.patch should be appliable over the others to make the javadocs more complete.

    I also made the Edge, Node, and DAG classes package private (by removing the "public" modifier).

    TestSignature doesn't extend CDKTest, but is that a problem? The only clear benefit I can see is logging, but I've not used logging...

     
  • Rajarshi Guha
    Rajarshi Guha
    2009-10-23

    Right, so patch 0001-Improved-javadocs-in-the-signature-package.patch
    should be appliable over the others to make the javadocs more complete.

    Looks good

    TestSignature doesn't extend CDKTest, but is that a problem? The only
    clear benefit I can see is logging, but I've not used logging...

    Not a problem per se. I just saw that all test classes were subclassing CDKTest and hence the comment. Egon might be better able to say if its needed.

    In any case, I think the patches are fine

     
  • Gilleain,

    I am about to upload a new .tar with 8 patches, rebased on the latest master, with my 4 patches in that list too... Can you please add the test class annotation? The coverage testing is reporting:

    signature.OrbitElement#compareTo does not have a test method
    signature.OrbitElement#compareTo does not have a test method
    signature.OrbitElement#toString does not have a test method
    signature.OrbitElement did not have a TestClass annotation
    signature.Signature#toCanonicalSignatureString does not have a test method
    signature.Signature#forAtom does not have a test method
    signature.Signature#forAtom does not have a test method
    signature.Signature#calculateOrbits does not have a test method
    signature.Signature#calculateOrbitElements does not have a test method
    signature.Signature#calculateOrbitElements does not have a test method
    signature.Signature#toMolecule does not have a test method
    signature.Signature#toMolecule does not have a test method
    signature.Signature#calculateOrbitElementsUnsorted does not have a test method
    signature.Signature#isCanonical does not have a test method
    signature.Signature#toCanonicalSignatureString has a test method annotation 'testIsCanonical' which is not found in test class: org.openscien
    ce.cdk.signature.TestSignature
    signature.Vertex#toString does not have a test method
    signature.Vertex did not have a TestClass annotation
    signature.DAG#get does not have a test method
    signature.DAG#iterator does not have a test method
    signature.DAG#size does not have a test method
    signature.DAG#getBestSignatureString does not have a test method
    signature.DAG#getMaxHeight does not have a test method
    signature.DAG#getMaxLabel does not have a test method
    signature.DAG#getRoot does not have a test method
    signature.DAG#getLargestLabel does not have a test method
    signature.DAG#getSMAX does not have a test method
    signature.DAG did not have a TestClass annotation
    signature.Edge#equals does not have a test method
    signature.Edge#toString does not have a test method
    signature.Edge did not have a TestClass annotation
    signature.TargetMolecularSignature#add does not have a test method
    signature.TargetMolecularSignature#add does not have a test method
    signature.TargetMolecularSignature#add does not have a test method
    signature.TargetMolecularSignature#toString does not have a test method
    signature.TargetMolecularSignature#size does not have a test method
    signature.TargetMolecularSignature#compatibleTargetBonds does not have a test method
    signature.TargetMolecularSignature#compatibleTargetBonds does not have a test method
    signature.TargetMolecularSignature#getCount does not have a test method
    signature.TargetMolecularSignature#getBondedSignatures does not have a test method
    signature.TargetMolecularSignature#getHeight does not have a test method
    signature.TargetMolecularSignature#getTargetAtomicSignature does not have a test method
    signature.TargetMolecularSignature#getTargetAtomicSignature does not have a test method
    signature.TargetMolecularSignature#getTargetAtomicSubSignature does not have a test method
    signature.TargetMolecularSignature did not have a TestClass annotation
    signature.Orbit#clone does not have a test method
    signature.Orbit#toString does not have a test method
    signature.Orbit#contains does not have a test method
    signature.Orbit#isEmpty does not have a test method
    signature.Orbit#iterator does not have a test method
    signature.Orbit#remove does not have a test method
    signature.Orbit#sort does not have a test method
    signature.Orbit#addAtom does not have a test method
    signature.Orbit#getFirstAtom does not have a test method
    signature.Orbit#getHeight does not have a test method
    signature.Orbit#getAtomIndices does not have a test method
    signature.Orbit#hasLabel does not have a test method
    signature.Orbit#getLabel does not have a test method
    signature.Orbit did not have a TestClass annotation
    signature.TargetAtomicSignature#toString does not have a test method
    signature.TargetAtomicSignature#toCanonicalSignatureString does not have a test method
    signature.TargetAtomicSignature#toMolecule does not have a test method
    signature.TargetAtomicSignature#toMolecule does not have a test method
    signature.TargetAtomicSignature#getSignatureStringsFromRootChildren does not have a test method
    signature.TargetAtomicSignature#getSignatureString does not have a test method
    signature.TargetAtomicSignature#getSubSignature does not have a test method
    signature.TargetAtomicSignature#getHeight does not have a test method
    signature.TargetAtomicSignature did not have a TestClass annotation

     
  • New full set of patches, rebased on master of 23 Nov.

     
    Attachments