#557 ChemModel/AtomContainerSet/ReactionSet/Crystal.isEmpty()

Accepted
closed
nobody
None
cdk-1.4.x
1
2012-10-31
2012-10-26
Ralf Stephan
No

Following feature request #184 here the implementation of isEmpty():

https://github.com/rwst/cdk/commit/e5d40a49b5d9e61acf469260f7ecb17c986a7377

Discussion

  • Ralf Stephan
    Ralf Stephan
    2012-10-27

    This should be set to "Needs review" which wasn't the default option BTW.

     
    • milestone: Accepted --> Needs_Review
     
  • Changed to 'Needs Review'

     
  • John May
    John May
    2012-10-28

    Sorry Ralf don't think I was clear.. the isEmpty() I was talking about was on the atom container. In the Sets I would just check if there are any atom containers (i.e. not if they are empty) - same with reaction. By bad. Will add comments on the branch.

    Why is this line there - the second part is only test if the first is true. So you only call isEmpty is the setOfMolecules is null -> NPE.

    if (setOfMolecules == null && !setOfMolecules.isEmpty())
    
     
  • John May
    John May
    2012-10-28

    Does this compile - I would have thought you need to implement the methods on default, silent and debug domain objects?

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-28

    The attached diff should address all issues. Thanks for your comments!

     
    Attachments
  • John May
    John May
    2012-10-28

    Looks good but I can't apply it - could you format the patch with git format-patch otherwise I can't use git am and lose your commit message. Also it would be good if you could add some unit tests to demonstrate the expected behaviour. We can add these after but it's smoother (and quicker) to do these all in one. Particularly add lots of corner cases for the ChemModel the isEmpty() there might have some unexpected behaviour. Also DebugRingSet, DebugAtomContainerSet etc. is missing the implementation.

    I would also be tempted to put this on master - I'm not sure how easy it is to patch forward any more.

    Thanks,
    J

     
  • John May
    John May
    2012-10-28

    grr... sourceforge

     
  • John May
    John May
    2012-10-28

    • milestone: Accepted --> Needs_Review
     
  • Ralf Stephan
    Ralf Stephan
    2012-10-30

    NOTE: The test was adapted to pass with the code in ChemModel.isEmpty(), so if you think the behaviour that is asserted there is wrong, then we have to change the isEmpty code too. That's also why I removed the Javadoc "if this ... has atoms"

     
  • John May
    John May
    2012-10-30

    Looks good - I think something got lost in the patch formatting:

    [javac] /Users/johnmay/workspace/github/johnmay/cdk/build/src/silent/org/openscience/cdk/silent/ChemModel.java:240: error: cannot find symbol
    [javac]         if (crystal != null && !crystal.isEmpty())
    [javac]                                        ^
    [javac]   symbol:   method isEmpty()
    [javac]   location: variable crystal of type ICrystal
    

    The interface method ICrystal.isEmpty was missing. I patched this up and also added the isEmpty to IAtomContainer interface (it's was already on the implementation) this also fixed ICrystal as ICrystal is an IAtomContainer subclass.

    Have pushed the signed commit to cdk-1.4.x-accepted

     
  • John May
    John May
    2012-10-30

    • milestone: Needs_Review --> Accepted
     
  • It needs a bit more work... When I run MdataTests, I get these missing unit tests:

    AtomContainerSet: missing the expected test method: testIsEmpty
    ReactionSet: missing the expected test method: testIsEmpty
    AtomContainer: missing the expected test method: testIsEmpty
    ChemModel: missing the expected test method: testIsEmpty

    Also, I had to add a missing interface method to ICrystal to get it to compile (attached).

     
  • John May
    John May
    2012-10-30

    Snap!

    I think we can just add the unit tests after - they're not complex and saves time actually getting it added.

    J

     
  • John May
    John May
    2012-10-30

    Actually this does need revision... just adding the unit tests and spotted

        /**
         * Returns true if this Crystal has no atoms.
         */
        public boolean isEmpty() {
            return aAxis == null;
        }
    
     
  • John May
    John May
    2012-10-30

    • milestone: Accepted --> Needs_Revision
     
  • John May
    John May
    2012-10-30

    Here's the complete unit tests. I also changed the Crystal isEmpty method to reflect what was documented - previously it was only checking the aAxis but now it just uses the super class isEmpty from AtomContainer.

    These applys after the commit on cdk-1.4.x-accepted

    Ralf/Egon please see if the changes/tests make sense and double check they pass

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-30

    OK, I've installed your cdk-1.4.x-accepted branch, your patch applies cleanly and all compiles without error. I've run the ChemModelTest, AtomContainerTest, ReactionSetTest, RingSetTest, CrystalTest without failure. If I knew before this was so involved I would have fixed three bugs in the meantime instead... but would have learned nothing. So many thanks for your help.

     
  • John May
    John May
    2012-10-30

    Thats no problem - you can also do the following

    ant -Dmodule=data test-module

    but you have to make sure you've done

    ant dist-all jarTestdata test-dist-all

    first.

    What did you think of the Crystal isEmpty()? I'm not sure what it should be...

    J

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-31

    Let the Crystal.isEmpty() as is, i.e., call the superclass' isEmpty(), IMHO. Anything else missing?

     
  • John May
    John May
    2012-10-31

    has been pushed

     
  • John May
    John May
    2012-10-31

    • status: open --> closed
    • milestone: Needs_Revision --> Accepted