#239 SMSD 1.2.0 code with updates

Needs_Review
closed
nobody
master (162)
7
2012-10-28
2010-07-30
Asad
No

SMSD core code moved as .jar
Main class is called Isomorphism.java
Bugs are fixed
IQueryAtomConatiner supported for SMARTS
Test class updated

Discussion

  • Asad
    Asad
    2010-08-05

    Here is a new set of patches without removing the core smsd lib.

     
  • Thanx!

     
  • I got a rejected hunk in MolHandler when applying the first patch, which I worked around. However, the following errors turned up in the report when running module test:

    Testcase: testSMSD(org.openscience.cdk.smsd.SMSDBondSensitiveTest): Caused an ERROR
    null
    java.lang.NullPointerException
    at org.openscience.cdk.smsd.SMSDBondSensitiveTest.testSMSD(SMSDBondSensitiveTest.java:155)

    Testcase: testSMSDBondSensitive(org.openscience.cdk.smsd.SMSDBondSensitiveTest): FAILED
    expected:<6> but was:<2>
    junit.framework.AssertionFailedError: expected:<6> but was:<2>
    at org.openscience.cdk.smsd.SMSDBondSensitiveTest.testSMSDBondSensitive(SMSDBondSensitiveTest.java:200)

    Testcase: testItShouldMatchHexaneToHexane(org.openscience.cdk.smsd.algorithm.vflib.VFLibTest): FAILED
    null
    junit.framework.AssertionFailedError: null
    at org.openscience.cdk.smsd.algorithm.vflib.VFLibTest.testItShouldMatchHexaneToHexane(VFLibTest.java:205)

    Testcase: testItShouldFindTwoMapsFromHexaneToHexane(org.openscience.cdk.smsd.algorithm.vflib.VFLibTest): FAILED
    expected:<6> but was:<0>
    junit.framework.AssertionFailedError: expected:<6> but was:<0>
    at org.openscience.cdk.smsd.algorithm.vflib.VFLibTest.testItShouldFindTwoMapsFromHexaneToHexane(VFLibTest.java:221)

    Testcase: testSFBug1208740(org.openscience.cdk.smsd.algorithm.rgraph.CDKMCSTest): FAILED
    expected:<8> but was:<9>
    junit.framework.AssertionFailedError: expected:<8> but was:<9>
    at org.openscience.cdk.smsd.algorithm.rgraph.CDKMCSTest.testSFBug1208740(CDKMCSTest.java:282)

    I've been working on the first of these, and it seems like the bonds never match (cylohexane to benzene) because the Is_Aromatic flag in the cyclohexane bonds are always false.

     
  • Asad
    Asad
    2010-08-06

    Thanks gilleain, I have updated the patches. please try again!

    Asad

     
  • Right. I still have the problem with MolHandler, but the errors reported by test-module are now just one:

    Testcase: testSFBug1208740(org.openscience.cdk.smsd.algorithm.rgraph.CDKMCSTest): FAILED
    expected:<8> but was:<9>
    junit.framework.AssertionFailedError: expected:<8> but was:<9>
    at org.openscience.cdk.smsd.algorithm.rgraph.CDKMCSTest.testSFBug1208740(CDKMCSTest.java:279)

    Which refers to a 'bug report' :

    https://sourceforge.net/tracker/index.php?func=detail&aid=1208740&group_id=20024&atid=120024

    that is a bit confusing, as it doesn't mention the specific test.

     
  • My comments (NT: next time, MF: must fix, SF: should/suggested fix, OF: optional fix).

    0001:
    NT: suggest to use <> around email in copyright header
    NT: better commit message: what changes can I expect in this commit. Major updates tells me nothing, and may cause random review comments later in this reply...

    • /**
      • {@inheritDoc}
      • @param Reactant
      • @param Product
    • */
    • @Override
    • @TestMethod("testInit_3args_1")
    • public void init(IMolecule Reactant, IMolecule Product, boolean removeHydrogen) throws CDKException {

    MF: Here you override the parameter descriptions with empty descriptions.
    SF: use lower case first chars for variables, per Java conventions. (for all methods)

    SF: public boolean isStereoMisMatch() has both @inheritDoc and an actual own JavaDoc.
    SF: same for isSubgraph()
    OF: bondA1 is quite cryptic
    OF: bondMatchFlag can perhaps be renamed? is it something like shouldMatchBonds?
    OF/NT: many private methods have no JavaDoc. Keep in mind that it helps other developers to understand the code if you would provide JavaDoc for all methods. Primarily, it is interesting to learn what assumptions you make, which is mostly less clear from the code.

         } catch (Exception ex) {
    
    • ex.printStackTrace();
    • ex.getCause();
      }

    SF: please add some comment here why this exception is not important and can be disregarded

    • /**
      • @return the bondMatch
    • */
    • public boolean isBondMatch() {
    • return bondMatch;
    • }
      +
    • /**
      • @param bondMatch the bondMatch to set
    • */
    • public void setBondMatch(boolean bondMatch) {
    • this.bondMatch = bondMatch;
    • }

    MF: missing JavaDoc. Please explain what this boolean controls.
    MF: I noted a few methods which are now 'public' with incomplete JavaDoc

    • private TimeManager timeManager = null;
    • private static TimeManager timeManager = null;

    Q: how will this affect the use in threads?

    • for (Map.Entry<IAtom, IAtom=""> map : solution.entrySet()) {
    • IAtom sourceAtom = map.getKey();
    • IAtom targetAtom = map.getValue();

    Cool! Did not know about that syntax. Very useful!

    0004:

    SF: IAtomMatcher.java needs a MX MIT license?
    MF/Q: does the final version have JavaDoc for the match method?

    0009:

    Q: can you explain why you made this patch:

     /** {@inheritDoc}
    
      • @param symbol

    0010:

    Nice new JavaDoc! Thanx!

    • Assert.assertEquals(3, instance.getAllMapping().size());
    • Assert.assertEquals(1, instance.getAllMapping().size());

    NT/Q: this is an excellent example of why good commit messages are important... why was this change made? was the unit test incorrect? What misassumption was made when it was decided that it should return 3? And why is the correct answer 1?

    0011:

    NT/Q: here the same as with 0010... you changed the outcome of unit tests, without explaining what was wrong with the previous test.

    0014:

    Excellent :) Some variables changed to start with lower cases, thanx!

    0017:

    • @Test
    • public void testItShouldMatchHexaneToHexane() {
    • IMapper mapper = new VFMapper(hexaneQuery);
    • Assert.assertTrue(mapper.hasMap(hexane));
    • }

    NT/Q: it should no longer match?

    0021:

    MF: another method that turned public, but without JavaDoc, and one or two new methods with incomplete JavaDoc. (Going through the patches in sequential order, so please disregard if already fixed in a later patch.)

    SF: here is a good example if the confusion I get from bondMatchFlag: the BondMatcher constructor takes this as parameter... I guess the whole point of the class is to match bonds... so, what does it mean if bondMatchFlag == false then??

    0023:

    NT: there is no need in signing off your own patches :)
    NT: "major algorithm update" ... oh, why? is it faster now?

    • System.out.println("\n IQueryAtomContainer Bond matches created");
    • System.out.println("\nthis.unsaturation " + this.unsaturation);

    MT: I assume this is removed later, otherwise, please do... if you must debug (which is often the case), use the ILoggingTool framework
    MT/Q: AbstractReactionLabeller.java has a copyright/license header now? and the other new classes too?

    if (atoms instanceof IPseudoAtom || atoms instanceof PseudoAtom) {

    SF: the first should be enough... (or otherwise, take into account DebugPseudoAtom and NNPseudoAtom too)
    OF: private boolean isPseudoAtoms() -> hasPseudoAtoms() or so?

    That's it. Overall, good patches! Better commit messages would be very much appreciated.

     
  • Asad
    Asad
    2010-08-10

    Hi,

    Thanks for the review. I have made implemented most of you suggestions.
    Please find the code patch attached.

     
  • Asad, which suggestions did you not implement then? And why not?

     
  • Asad
    Asad
    2010-08-16

    My comments (NT: next time, MF: must fix, SF: should/suggested fix, OF:
    optional fix).

    0001:
    NT: suggest to use <> around email in copyright header

    DONE

    NT: better commit message: what changes can I expect in this commit. Major
    updates tells me nothing, and may cause random review comments later in
    this reply...

    DONE

    • /**
      • {@inheritDoc}
      • @param Reactant
      • @param Product
    • */
    • @Override
    • @TestMethod("testInit_3args_1")
    • public void init(IMolecule Reactant, IMolecule Product, boolean
      removeHydrogen) throws CDKException {

    MF: Here you override the parameter descriptions with empty descriptions.
    SF: use lower case first chars for variables, per Java conventions. (for
    all methods)

    DONE

    SF: public boolean isStereoMisMatch() has both @inheritDoc and an actual
    own JavaDoc.
    DONE
    SF: same for isSubgraph()
    DONE


    OF: bondA1 is quite cryptic

    Not DONE

    OF: bondMatchFlag can perhaps be renamed? is it something like
    shouldMatchBonds?
    DONE

    OF/NT: many private methods have no JavaDoc. Keep in mind that it helps
    other developers to understand the code if you would provide JavaDoc for
    all methods. Primarily, it is interesting to learn what assumptions you
    make, which is mostly less clear from the code.

    DONE but might have missed few

    } catch (Exception ex) {
    - ex.printStackTrace();
    + ex.getCause();
    }

    SF: please add some comment here why this exception is not important and
    can be disregarded

    • /**
      • @return the bondMatch
    • */
    • public boolean isBondMatch() {
    • return bondMatch;
    • }
      +
    • /**
      • @param bondMatch the bondMatch to set
    • */
    • public void setBondMatch(boolean bondMatch) {
    • this.bondMatch = bondMatch;
    • }

    DONE

    MF: missing JavaDoc. Please explain what this boolean controls.
    MF: I noted a few methods which are now 'public' with incomplete JavaDoc

    • private TimeManager timeManager = null;
    • private static TimeManager timeManager = null;

    DONE

    Q: how will this affect the use in threads?

    Not sure!

    • for (Map.Entry<IAtom, IAtom=""> map : solution.entrySet())
      {
    • IAtom sourceAtom = map.getKey();
    • IAtom targetAtom = map.getValue();

    Cool! Did not know about that syntax. Very useful!

    0004:

    SF: IAtomMatcher.java needs a MX MIT license?
    MF/Q: does the final version have JavaDoc for the match method?

    DONE

    0009:

    Q: can you explain why you made this patch:

    /** {@inheritDoc}
    + * @param symbol

    0010:

    Nice new JavaDoc! Thanx!

    • Assert.assertEquals(3, instance.getAllMapping().size());
    • Assert.assertEquals(1, instance.getAllMapping().size());

    NT/Q: this is an excellent example of why good commit messages are
    important... why was this change made? was the unit test incorrect? What
    misassumption was made when it was decided that it should return 3? And why
    is the correct answer 1?

    DONE, these changes are related to the changes in the core code, any updating is always towards perfection but sometimes you might land in spikes.

    0011:

    NT/Q: here the same as with 0010... you changed the outcome of unit tests,
    without explaining what was wrong with the previous test.

    DONE, refactoring of the SMSD

    0014:

    Excellent :) Some variables changed to start with lower cases, thanx!

    Thanks

    0017:

    • @Test
    • public void testItShouldMatchHexaneToHexane() {
    • IMapper mapper = new VFMapper(hexaneQuery);
    • Assert.assertTrue(mapper.hasMap(hexane));
    • }

    NT/Q: it should no longer match?

    DONE, refactoring

    0021:

    MF: another method that turned public, but without JavaDoc, and one or two
    new methods with incomplete JavaDoc. (Going through the patches in
    sequential order, so please disregard if already fixed in a later patch.)

    DONE

    SF: here is a good example if the confusion I get from bondMatchFlag: the
    BondMatcher constructor takes this as parameter... I guess the whole point
    of the class is to match bonds... so, what does it mean if bondMatchFlag ==
    false then??

    I think the code is working fine, I am not sure whats your point?

    0023:

    NT: there is no need in signing off your own patches :)
    NT: "major algorithm update" ... oh, why? is it faster now?

    • System.out.println("\n IQueryAtomContainer Bond matches
      created");
    • System.out.println("\nthis.unsaturation " + this.unsaturation);

    Faster and more organised

    MT: I assume this is removed later, otherwise, please do... if you must
    debug (which is often the case), use the ILoggingTool framework

    Good idea

    MT/Q: AbstractReactionLabeller.java has a copyright/license header now?
    and the other new classes too?

    correct...

    if (atoms instanceof IPseudoAtom || atoms instanceof PseudoAtom) {

    SF: the first should be enough... (or otherwise, take into account
    DebugPseudoAtom and NNPseudoAtom too)

    thanks will do that in the next patch

    OF: private boolean isPseudoAtoms() -> hasPseudoAtoms() or so?

    Will do that in the next patch,

    That's it. Overall, good patches! Better commit messages would be very
    much appreciated.

     
  • Reviewed. Compiles ok, though there may be an issue with the first patch and MolHandler - not a problem, since many classes are (re)moved in 0001, so it can just be removed separately.

    One test case fails, but this is due to the CDK, not SMSD code.

     
  • Asad
    Asad
    2010-08-25

    VFLib speed up patch added

     
  • Asad
    Asad
    2010-08-29

    Compressed patch1

     
    Attachments
  • Asad
    Asad
    2010-08-29

    Compressed patch2

     
    Attachments
  • Asad, I cannot accept these STEP1 patches... really... if you remove everything and then add it again, there is no way I can review those things... you've relayout a very large part of build.xml (patch 0003)...

    I have attached a collapsed 0001 patch for STEP1, without patch 0003. Please attach a new STEP1.0003 patch with only the really required change, without all the whitespace changes. Please also verify patch 0001.

    Your patches in STEP2 are a bit cryptic, and particularly unclear to which reviewer comments they are in reply... or are they completely new, and thus need review? Again, please do put in the commit message why you change unit tests' expected output... I see recent commits changes expected values from false to true and back to false? Please explain on the commit messages what is wrong with those unit tests... it makes me really wary of the whole SMSD code base if something is sometimes a substructure and then it is not, but no wait, it is... does not really make me trust the code base.

    So, for now: please fix that patch 0003, review the collapsed patch I'll attach, and then things are good to go.