From: Egon W. <ego...@gm...> - 2008-10-19 19:14:28
|
On Sun, Oct 19, 2008 at 9:12 PM, Rajarshi Guha <rg...@in...> wrote: > On Oct 19, 2008, at 3:05 PM, Egon Willighagen wrote: >> https://apps.sourceforge.net/mediawiki/cdk/index.php?title=CDK_1.2_TODO > > I'd also point out that a number of bugs on the list refer to 1.0 and have > been fixed in trunk (and 1.2.x). I haven't closed some of them, since the > data files don't work. But I don't think they are worth fixing, given that > we are going to be putting out 1.2 Please be more specific and list the bug numbers. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Egon W. <ego...@gm...> - 2008-10-19 19:16:54
|
On Sun, Oct 19, 2008 at 9:10 PM, Rajarshi Guha <rg...@in...> wrote: > Certainly, more tests is not harmful! The final count I have here is 14124... of which ~8000 are for the data classes... > BTW, I think it's important that coverage tests be converted to > CoverageAnnotationTest - this is because the latter will identify whether > methods of a class not explicitly marked as tested, are actually tested in a > superclass. Which means that if I subclass a class and overwrite a method, I do not have to 'overwrite' the test annotation too? Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-10-19 19:47:21
|
On Oct 19, 2008, at 3:16 PM, Egon Willighagen wrote: > On Sun, Oct 19, 2008 at 9:10 PM, Rajarshi Guha <rg...@in...> > wrote: >> >> BTW, I think it's important that coverage tests be converted to >> CoverageAnnotationTest - this is because the latter will identify >> whether >> methods of a class not explicitly marked as tested, are actually >> tested in a >> superclass. > > Which means that if I subclass a class and overwrite a method, I do > not have to 'overwrite' the test annotation too? Yes (I think). Basaically given a super class: class Atest { public void someCommonMethod() } class Btest extends Atest { public void someOtherMethod() } @TestClass("Btest") class MyClass { @TestMethd("someCommonMethod") public void someCommonMethod() { ... } ... } So even though someCommonMethod is not tested specifically in Btest, the coverge annotation test knows to look in the super class of Btest for that specific test method Is this what you meant? ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- A motion to adjourn is always in order. |
From: Egon W. <ego...@gm...> - 2008-10-19 19:27:51
|
On Sun, Oct 19, 2008 at 9:23 PM, Rajarshi Guha <rg...@in...> wrote: > On Oct 19, 2008, at 3:14 PM, Egon Willighagen wrote: >> Please be more specific and list the bug numbers. > > 2032597 - > https://sourceforge.net/tracker2/?func=detail&aid=2032597&group_id=20024&atid=120024 > > 1955638 - > https://sourceforge.net/tracker2/?func=detail&aid=1955638&group_id=20024&atid=120024 > > 2031509 - > https://sourceforge.net/tracker2/?func=detail&aid=2031509&group_id=20024&atid=120024 > > Also, the following bug was posted on the list for discussion, but nobody > replied. I think this can be a major source of inconsistency and needs to be > addressed, ASAP. > > 2101109 - > https://sourceforge.net/tracker2/?func=detail&aid=2101109&group_id=20024&atid=120024 OK, please mark them as CDK 1.2.x. As they are release blockers, please mark them priority 9. > The atom typing overwriting bug is easy to fix - just needs to be discussed > and implemented. Since we are in freeze, I'd welcome clear JavaDoc for the current method, and possible an extra method for what we decided we really wanted. > The VdW bug is also easy to fix - we just need to agree on > whether to use covalent radii (atom type dependent) or VdW radii (atom type > independent) This was for the TPSA, not? You suggested to use VdW, right? Sounded reasonable. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Egon W. <ego...@gm...> - 2008-10-19 19:36:25
|
On Sun, Oct 19, 2008 at 9:28 PM, Rajarshi Guha <rg...@in...> wrote: > Is this what you meant? No, don't think so :) I was wondering about: public class A { @TestMethod("testMethod") public void method(); } public class B extends A { public void method(); } Would CoverageAnnotationTest recognize B.method() to be overwriting A.method() and pick the TestMethod annotation from A instead? Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-10-19 19:39:10
|
On Oct 19, 2008, at 3:27 PM, Egon Willighagen wrote: > > OK, please mark them as CDK 1.2.x. As they are release blockers, > please mark them priority 9. Done >> The atom typing overwriting bug is easy to fix - just needs to be >> discussed >> and implemented. > > Since we are in freeze, I'd welcome clear JavaDoc for the current > method, and possible an extra method for what we decided we really > wanted. If we're adding another method, why not rather fix the perceiveAndCnfigureAtoms method? I think I added a method that clears atom type configuration for a given atom (i.e., the reverse of the percieve method) >> The VdW bug is also easy to fix - we just need to agree on >> whether to use covalent radii (atom type dependent) or VdW radii >> (atom type >> independent) > > This was for the TPSA, not? You suggested to use VdW, right? > Sounded reasonable. This was for lengthOverBreadth - and I agree that using VdW radii is fine but not exactly correct. I've never considered this descriptor to be very discriminating, so maybe an approximation to the extents does not matter too much. ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- In matrimony, to hesitate is sometimes to be saved. -- Butler |
From: Egon W. <ego...@gm...> - 2008-10-19 19:42:27
|
On Sun, Oct 19, 2008 at 9:38 PM, Rajarshi Guha <rg...@in...> wrote: > On Oct 19, 2008, at 3:27 PM, Egon Willighagen wrote: >> Since we are in freeze, I'd welcome clear JavaDoc for the current >> method, and possible an extra method for what we decided we really >> wanted. > > If we're adding another method, why not rather fix the > perceiveAndCnfigureAtoms method? I think I added a method that clears > atom type configuration for a given atom (i.e., the reverse of the > percieve method) Because I expect it to do what it currently is doing... and we can likely expect bugs if we change behavior now. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-10-19 19:43:22
|
On Oct 19, 2008, at 3:36 PM, Egon Willighagen wrote: > On Sun, Oct 19, 2008 at 9:28 PM, Rajarshi Guha <rg...@in...> > wrote: >> Is this what you meant? > > No, don't think so :) > > I was wondering about: > > public class A { > > @TestMethod("testMethod") > public void method(); > > } > > public class B extends A { > > public void method(); > > } > > Would CoverageAnnotationTest recognize B.method() to be overwriting > A.method() and pick the TestMethod annotation from A instead? Aah. No - you have to explicitly specify a test method/class name. What you describe above, would be against the original idea of using test method / class annotations, since we want to force that each source method is explicitly annotated. Rather, the current approach saves a little time when implementing the tests - for example, in the descriptor and IO classes, there are some methods that are only defined in the superclass and not over- ridden in the child class - but the coverage test will see them in the child class and complain that they are not annotated/tested. This way, as long as these methods are tested in the super class, the coverage tester is OK ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- A memorandum is written not to inform the reader, but to protect the writer. -- Dean Acheson |
From: Egon W. <ego...@gm...> - 2008-10-19 19:45:52
|
On Sun, Oct 19, 2008 at 9:43 PM, Rajarshi Guha <rg...@in...> wrote: > What you describe above, would be against the original idea of using test > method / class annotations, since we want to force that each source method > is explicitly annotated. Indeed. Should have made that link... OK, will be back tomorrow. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-10-19 19:46:53
|
On Oct 19, 2008, at 3:42 PM, Egon Willighagen wrote: > On Sun, Oct 19, 2008 at 9:38 PM, Rajarshi Guha <rg...@in...> > wrote: >> On Oct 19, 2008, at 3:27 PM, Egon Willighagen wrote: >>> Since we are in freeze, I'd welcome clear JavaDoc for the current >>> method, and possible an extra method for what we decided we really >>> wanted. >> >> If we're adding another method, why not rather fix the >> perceiveAndCnfigureAtoms method? I think I added a method that clears >> atom type configuration for a given atom (i.e., the reverse of the >> percieve method) > > Because I expect it to do what it currently is doing... and we can > likely expect bugs if we change behavior now. But I think the problem is significant enough: Load SD file - atoms have an exact mass Configure atoms - now atoms do no have an exact mass How does the user get back exact masses, that were provided when the file was loaded in? ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- Regular naps prevent old age.... especially if you take them while driving |
From: Egon W. <ego...@gm...> - 2008-10-20 06:05:06
|
On Sun, Oct 19, 2008 at 9:46 PM, Rajarshi Guha <rg...@in...> wrote: > On Oct 19, 2008, at 3:42 PM, Egon Willighagen wrote: >> On Sun, Oct 19, 2008 at 9:38 PM, Rajarshi Guha <rg...@in...> >> wrote: >> Because I expect it to do what it currently is doing... and we can >> likely expect bugs if we change behavior now. > > But I think the problem is significant enough: > > Load SD file - atoms have an exact mass > Configure atoms - now atoms do no have an exact mass When, where is this configuration done? Information from files should certainly not be lost upon reading that file. OK, please commit a full patch to trunk for the 'bug' and I'll review... Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-10-20 14:59:33
|
On Oct 20, 2008, at 2:04 AM, Egon Willighagen wrote: > On Sun, Oct 19, 2008 at 9:46 PM, Rajarshi Guha <rg...@in...> > wrote: >> On Oct 19, 2008, at 3:42 PM, Egon Willighagen wrote: >>> On Sun, Oct 19, 2008 at 9:38 PM, Rajarshi Guha <rg...@in...> >>> wrote: >>> Because I expect it to do what it currently is doing... and we can >>> likely expect bugs if we change behavior now. >> >> But I think the problem is significant enough: >> >> Load SD file - atoms have an exact mass >> Configure atoms - now atoms do no have an exact mass > > When, where is this configuration done? If I call percieveAndConfigureAtoms after the molecule is loaded from the file. See the new unit test in AtomContainerManipulatorTest > Information from files should certainly not be lost upon reading > that file. > > OK, please commit a full patch to trunk for the 'bug' and I'll > review... Patch (for trunk) uploaded to https://sourceforge.net/tracker2/? func=detail&aid=1969156&group_id=20024&atid=120024 ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- Brain fried -- Core dumped |
From: Egon W. <ego...@gm...> - 2008-10-20 15:13:48
|
On Mon, Oct 20, 2008 at 4:59 PM, Rajarshi Guha <rg...@in...> wrote: >> OK, please commit a full patch to trunk for the 'bug' and I'll >> review... > > Patch (for trunk) uploaded to > > https://sourceforge.net/tracker2/? > func=detail&aid=1969156&group_id=20024&atid=120024 Thanx Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-10-19 19:47:30
|
On Oct 19, 2008, at 3:14 PM, Egon Willighagen wrote: > On Sun, Oct 19, 2008 at 9:12 PM, Rajarshi Guha <rg...@in...> > wrote: >> On Oct 19, 2008, at 3:05 PM, Egon Willighagen wrote: >>> https://apps.sourceforge.net/mediawiki/cdk/index.php? >>> title=CDK_1.2_TODO >> >> I'd also point out that a number of bugs on the list refer to 1.0 >> and have >> been fixed in trunk (and 1.2.x). I haven't closed some of them, >> since the >> data files don't work. But I don't think they are worth fixing, >> given that >> we are going to be putting out 1.2 > > Please be more specific and list the bug numbers. 2032597 - https://sourceforge.net/tracker2/? func=detail&aid=2032597&group_id=20024&atid=120024 1955638 - https://sourceforge.net/tracker2/? func=detail&aid=1955638&group_id=20024&atid=120024 2031509 - https://sourceforge.net/tracker2/? func=detail&aid=2031509&group_id=20024&atid=120024 Also, the following bug was posted on the list for discussion, but nobody replied. I think this can be a major source of inconsistency and needs to be addressed, ASAP. 2101109 - https://sourceforge.net/tracker2/? func=detail&aid=2101109&group_id=20024&atid=120024 The atom typing overwriting bug is easy to fix - just needs to be discussed and implemented. The VdW bug is also easy to fix - we just need to agree on whether to use covalent radii (atom type dependent) or VdW radii (atom type independent) ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- A computer lets you make more mistakes faster than any other invention, with the possible exceptions of handguns and Tequilla. -- Mitch Ratcliffe |