From: SourceForge.net <no...@so...> - 2010-08-25 08:17:39
|
Patches item #3037137, was opened at 2010-07-30 15:43 Message generated for change (Comment added) made by asadrahman You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3037137&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: master Group: Needs Review Status: Open Resolution: None Priority: 7 Private: No Submitted By: Asad (asadrahman) Assigned to: Nobody/Anonymous (nobody) Summary: SMSD 1.0.2 code with updates Initial Comment: SMSD core code moved as .jar Main class is called Isomorphism.java Bugs are fixed IQueryAtomConatiner supported for SMARTS Test class updated ---------------------------------------------------------------------- >Comment By: Asad (asadrahman) Date: 2010-08-25 09:17 Message: VFLib speed up patch added ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2010-08-16 15:03 Message: 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. ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2010-08-16 11:56 Message: 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. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-08-13 13:23 Message: Asad, which suggestions did you not implement then? And why not? ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2010-08-10 23:24 Message: Hi, Thanks for the review. I have made implemented most of you suggestions. Please find the code patch attached. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-08-08 14:19 Message: 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. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2010-08-08 12:54 Message: Oh, I found an email from Irilenia Nobeli on this: https://sourceforge.net/mailarchive/forum.php?thread_name=Pine.LNX.4.44.0506151947560.2289-100000%40ginger.ebi.ac.uk&forum_name=cdk-user Which suggests the problem is to do with doing aromaticity detection multiple times, and flags not getting cleared properly... ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2010-08-08 12:44 Message: 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. ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2010-08-06 22:37 Message: Thanks gilleain, I have updated the patches. please try again! Asad ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2010-08-06 18:20 Message: 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. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-08-05 11:33 Message: Thanx! ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2010-08-05 11:09 Message: Here is a new set of patches without removing the core smsd lib. ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2010-08-02 12:55 Message: kindly download the patches from http://github.com/downloads/asad/cdk/patches.zip ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2010-07-30 15:50 Message: Patches can be download from http://github.com/downloads/asad/cdk/patches.zip as they are very big. SF refuses to lead them! ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3037137&group_id=20024 |