From: SourceForge.net <no...@so...> - 2009-08-18 09:05:05
|
Patches item #2839472, was opened at 2009-08-18 11:05 Message generated for change (Tracker Item Submitted) made by shk3 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&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: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Stefan Kuhn (shk3) Assigned to: Nobody/Anonymous (nobody) Summary: Patches for CrossoverMachine and PartialFilledStructureMerge Initial Comment: Please find attached new implementations for CrossoverMachine and PartialFilledStructureMerger and tests for them. Both classes were untested and had no chance of ever working. The CrossoverMachine could not return any result, as one could see from a short glance at the code. The PartialFilledStructureMerger was a near copy of the SingleStructureRandomGenerator and therefore also not doing what it was supposed to do. Both classes are largely rewritten from scratch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-08-18 11:13:22
|
Patches item #2839472, was opened at 2009-08-18 11:05 Message generated for change (Comment added) made by shk3 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&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: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Stefan Kuhn (shk3) Assigned to: Nobody/Anonymous (nobody) Summary: Patches for CrossoverMachine and PartialFilledStructureMerge Initial Comment: Please find attached new implementations for CrossoverMachine and PartialFilledStructureMerger and tests for them. Both classes were untested and had no chance of ever working. The CrossoverMachine could not return any result, as one could see from a short glance at the code. The PartialFilledStructureMerger was a near copy of the SingleStructureRandomGenerator and therefore also not doing what it was supposed to do. Both classes are largely rewritten from scratch. ---------------------------------------------------------------------- >Comment By: Stefan Kuhn (shk3) Date: 2009-08-18 13:13 Message: CrossoverMachineTest: * explain why it is reasonable for the algorithm to fail up to 299 times? (and add comments to the source accordingly...) Basically, it is reasonable because the old one failed 10000 times out of 10000 times. 300 seems like an improvment for me. * since this is new functionality, please add full unit testing This should be there * run the PMD testing on structgen and test-structgen and please fix the fails for CrossoverMachine and PartialFilledStructureMerger and tests (~7 fails in your new code in structgen) The only pmd messages I get relate to complexity etc. There is a lot of code with such messages and considering that the old code did not work at all, I don't feel like bothering about this right now. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-08-18 11:21:53
|
Patches item #2839472, was opened at 2009-08-18 11:05 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&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: None >Group: Needs Revision Status: Open Resolution: None Priority: 5 Private: No Submitted By: Stefan Kuhn (shk3) Assigned to: Nobody/Anonymous (nobody) Summary: Patches for CrossoverMachine and PartialFilledStructureMerge Initial Comment: Please find attached new implementations for CrossoverMachine and PartialFilledStructureMerger and tests for them. Both classes were untested and had no chance of ever working. The CrossoverMachine could not return any result, as one could see from a short glance at the code. The PartialFilledStructureMerger was a near copy of the SingleStructureRandomGenerator and therefore also not doing what it was supposed to do. Both classes are largely rewritten from scratch. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2009-08-18 13:21 Message: Thanks for adding the unit test. The other two comments I made, I do not find addressed sufficiently. * Please explain what the source of those 299 fails are, and how that affects the use of the algorithm. * The old code was written before the project instantiated those rules. New code should abide them, and I think you are qualified enough to write clean code... your argument for not doing so, is not good enough for me. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-08-18 13:13 Message: CrossoverMachineTest: * explain why it is reasonable for the algorithm to fail up to 299 times? (and add comments to the source accordingly...) Basically, it is reasonable because the old one failed 10000 times out of 10000 times. 300 seems like an improvment for me. * since this is new functionality, please add full unit testing This should be there * run the PMD testing on structgen and test-structgen and please fix the fails for CrossoverMachine and PartialFilledStructureMerger and tests (~7 fails in your new code in structgen) The only pmd messages I get relate to complexity etc. There is a lot of code with such messages and considering that the old code did not work at all, I don't feel like bothering about this right now. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-08-26 12:31:29
|
Patches item #2839472, was opened at 2009-08-18 11:05 Message generated for change (Comment added) made by shk3 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&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: None Group: Needs Revision Status: Open Resolution: None Priority: 5 Private: No Submitted By: Stefan Kuhn (shk3) Assigned to: Nobody/Anonymous (nobody) Summary: Patches for CrossoverMachine and PartialFilledStructureMerge Initial Comment: Please find attached new implementations for CrossoverMachine and PartialFilledStructureMerger and tests for them. Both classes were untested and had no chance of ever working. The CrossoverMachine could not return any result, as one could see from a short glance at the code. The PartialFilledStructureMerger was a near copy of the SingleStructureRandomGenerator and therefore also not doing what it was supposed to do. Both classes are largely rewritten from scratch. ---------------------------------------------------------------------- >Comment By: Stefan Kuhn (shk3) Date: 2009-08-26 14:31 Message: Fixing the complexity shouldn't be a problem, but the other stuff is. If I would know what exactly are the reasons for failure it would not fail. The judge use of the algorithm is up to the user, I would think. For example, in many cases, you might just want to use the parents unchanged. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-08-18 13:21 Message: Thanks for adding the unit test. The other two comments I made, I do not find addressed sufficiently. * Please explain what the source of those 299 fails are, and how that affects the use of the algorithm. * The old code was written before the project instantiated those rules. New code should abide them, and I think you are qualified enough to write clean code... your argument for not doing so, is not good enough for me. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-08-18 13:13 Message: CrossoverMachineTest: * explain why it is reasonable for the algorithm to fail up to 299 times? (and add comments to the source accordingly...) Basically, it is reasonable because the old one failed 10000 times out of 10000 times. 300 seems like an improvment for me. * since this is new functionality, please add full unit testing This should be there * run the PMD testing on structgen and test-structgen and please fix the fails for CrossoverMachine and PartialFilledStructureMerger and tests (~7 fails in your new code in structgen) The only pmd messages I get relate to complexity etc. There is a lot of code with such messages and considering that the old code did not work at all, I don't feel like bothering about this right now. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-08-26 13:20:23
|
Patches item #2839472, was opened at 2009-08-18 11:05 Message generated for change (Comment added) made by steinbeck You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&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: None Group: Needs Revision Status: Open Resolution: None Priority: 5 Private: No Submitted By: Stefan Kuhn (shk3) Assigned to: Nobody/Anonymous (nobody) Summary: Patches for CrossoverMachine and PartialFilledStructureMerge Initial Comment: Please find attached new implementations for CrossoverMachine and PartialFilledStructureMerger and tests for them. Both classes were untested and had no chance of ever working. The CrossoverMachine could not return any result, as one could see from a short glance at the code. The PartialFilledStructureMerger was a near copy of the SingleStructureRandomGenerator and therefore also not doing what it was supposed to do. Both classes are largely rewritten from scratch. ---------------------------------------------------------------------- >Comment By: Christoph Steinbeck (steinbeck) Date: 2009-08-26 15:20 Message: I assume that failure means that the algorithm is not able to saturate the incomplete structure property. We have had this problem in all earlier versions of this algorithm and unfortunately I'm not aware of any algorithm that guarantees to saturate the structure properly in all cases. That means: we can either remove this package from the chemistry development Kit completely or we document the behavior and leave it in. Personally, I would prefer the latter. Having a genetic algorithm optimization in the CDK is important and it might attract users with more experience who might even be able to fix the problem. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-08-26 14:31 Message: Fixing the complexity shouldn't be a problem, but the other stuff is. If I would know what exactly are the reasons for failure it would not fail. The judge use of the algorithm is up to the user, I would think. For example, in many cases, you might just want to use the parents unchanged. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-08-18 13:21 Message: Thanks for adding the unit test. The other two comments I made, I do not find addressed sufficiently. * Please explain what the source of those 299 fails are, and how that affects the use of the algorithm. * The old code was written before the project instantiated those rules. New code should abide them, and I think you are qualified enough to write clean code... your argument for not doing so, is not good enough for me. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-08-18 13:13 Message: CrossoverMachineTest: * explain why it is reasonable for the algorithm to fail up to 299 times? (and add comments to the source accordingly...) Basically, it is reasonable because the old one failed 10000 times out of 10000 times. 300 seems like an improvment for me. * since this is new functionality, please add full unit testing This should be there * run the PMD testing on structgen and test-structgen and please fix the fails for CrossoverMachine and PartialFilledStructureMerger and tests (~7 fails in your new code in structgen) The only pmd messages I get relate to complexity etc. There is a lot of code with such messages and considering that the old code did not work at all, I don't feel like bothering about this right now. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&group_id=20024 |
From: SourceForge.net <no...@so...> - 2010-02-27 11:28:31
|
Patches item #2839472, was opened at 2009-08-18 11:05 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&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: None >Group: Accepted >Status: Closed >Resolution: Fixed Priority: 5 Private: No Submitted By: Stefan Kuhn (shk3) Assigned to: Nobody/Anonymous (nobody) Summary: Patches for CrossoverMachine and PartialFilledStructureMerge Initial Comment: Please find attached new implementations for CrossoverMachine and PartialFilledStructureMerger and tests for them. Both classes were untested and had no chance of ever working. The CrossoverMachine could not return any result, as one could see from a short glance at the code. The PartialFilledStructureMerger was a near copy of the SingleStructureRandomGenerator and therefore also not doing what it was supposed to do. Both classes are largely rewritten from scratch. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2010-02-27 12:28 Message: Chris, thanx for backing up the patches as original author of the code. I fixed a missing dependency for the test module, and applied things to CDK master. I would like to request to look at the unit test coverage testing, and notice these errors: CrossoverMachine: missing the expected test method: testDoCrossover_IAtomContainer_IAtomContainer CrossoverMachine: missing the expected test method: testCrossoverMachine PartialFilledStructureMerger: missing the expected test method: testPartialFilledStructureMerger Use the old naming scheme approach or the newer @TestClass/TestMethod annotation to address it. Other suggestions: * refer in the JavaDoc to the paper where the method is described * write unit tests for ChemGraph * write CrossoverMachine unit tests for testing that the bond shuffling works as expected ( * have a look at http://pele.farmbio.uu.se/nightly/ojdcheck/structgen.html (wrt to the two classes) * have a look at http://pele.farmbio.uu.se/nightly/pmd/structgen.html (wrt to the two classes) ---------------------------------------------------------------------- Comment By: Christoph Steinbeck (steinbeck) Date: 2009-08-26 15:20 Message: I assume that failure means that the algorithm is not able to saturate the incomplete structure property. We have had this problem in all earlier versions of this algorithm and unfortunately I'm not aware of any algorithm that guarantees to saturate the structure properly in all cases. That means: we can either remove this package from the chemistry development Kit completely or we document the behavior and leave it in. Personally, I would prefer the latter. Having a genetic algorithm optimization in the CDK is important and it might attract users with more experience who might even be able to fix the problem. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-08-26 14:31 Message: Fixing the complexity shouldn't be a problem, but the other stuff is. If I would know what exactly are the reasons for failure it would not fail. The judge use of the algorithm is up to the user, I would think. For example, in many cases, you might just want to use the parents unchanged. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-08-18 13:21 Message: Thanks for adding the unit test. The other two comments I made, I do not find addressed sufficiently. * Please explain what the source of those 299 fails are, and how that affects the use of the algorithm. * The old code was written before the project instantiated those rules. New code should abide them, and I think you are qualified enough to write clean code... your argument for not doing so, is not good enough for me. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-08-18 13:13 Message: CrossoverMachineTest: * explain why it is reasonable for the algorithm to fail up to 299 times? (and add comments to the source accordingly...) Basically, it is reasonable because the old one failed 10000 times out of 10000 times. 300 seems like an improvment for me. * since this is new functionality, please add full unit testing This should be there * run the PMD testing on structgen and test-structgen and please fix the fails for CrossoverMachine and PartialFilledStructureMerger and tests (~7 fails in your new code in structgen) The only pmd messages I get relate to complexity etc. There is a lot of code with such messages and considering that the old code did not work at all, I don't feel like bothering about this right now. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2839472&group_id=20024 |