From: Stefan K. <ste...@eb...> - 2009-07-31 17:22:56
|
Please find attached a number of patches. They add a new bond type (wiggly==undefined E/Z) and extend the old undefined stereo to be drawn in both directions, as the other wedges can be. These bond types are IUPAC recommendations. Note the wiggly bond is not yet drawn in the SVG visitor (there is a bug report about that). Stefan -- Stefan Kuhn B. Sc. M. A. Software Engineer in the Chemoinformatics and Metabolism Team European Bioinformatics Institute (EBI) Wellcome Trust Genome Campus Hinxton, Cambridge CB10 1SD UK Phone +44 1223 49 2657 Fax +44 (0)1223 494 468 |
From: Egon W. <ego...@gm...> - 2009-07-31 18:48:33
|
Hi Stefan, On Fri, Jul 31, 2009 at 7:22 PM, Stefan Kuhn<ste...@eb...> wrote: > Please find attached a number of patches. They add a new bond type > (wiggly==undefined E/Z) and extend the old undefined stereo to be drawn in > both directions, as the other wedges can be. These bond types are IUPAC > recommendations. I have a look at your code, and have these comments: * why is there need to change the type WedgeLineElement.isDashed? I only see use of ==0 and ==1... and if this is really needed, I strongly suggest to rename the variable and/or to use an enum. * WigglyLineElement is, I assume, copyright 2009, not 2008 * WigglyLineElement misses JavaDoc on the class and methods. The class JavaDoc would also be the place to refer to the IUPAC recommendations, for which @cdk.cite should be used. * I would like to encourage you to use 80 characters per line * please add JavaDoc to methods you have changed API and/or have rewritten, viz. BasicBondGenerator.generateStereoElement() and the WedgeLineElement constructors > Note the wiggly bond is not yet drawn in the SVG visitor > (there is a bug report about that). Please annotate the class with @cdk.bug accordingly. Egon -- Post-doc @ Uppsala University http://chem-bla-ics.blogspot.com/ |
From: Stefan K. <ste...@eb...> - 2009-08-04 09:09:33
Attachments:
0001-added-everything-needed-for-drawing-wiggle-and-criss.patch
0001-added-more-stereo-bond-types.patch
0001-comments-in-BasicBondGenerator.patch
0001-added-wiggly-line-to-elements-comments-for-wedge-li.patch
0001-AWTVisitor-visits-wiggly-bonds.patch
0001-svg-visitor-visits-crisscrossbonds.patch
0002-added-everything-needed-for-drawing-wiggle-and-criss.patch
|
On Friday 31 July 2009 19:48:00 Egon Willighagen wrote: > Hi Stefan, > > On Fri, Jul 31, 2009 at 7:22 PM, Stefan Kuhn<ste...@eb...> wrote: > > Please find attached a number of patches. They add a new bond type > > (wiggly==undefined E/Z) and extend the old undefined stereo to be drawn > > in both directions, as the other wedges can be. These bond types are > > IUPAC recommendations. > > I have a look at your code, and have these comments: > > * why is there need to change the type WedgeLineElement.isDashed? I > only see use of ==0 and ==1... and if this is really needed, I > strongly suggest to rename the variable and/or to use an enum. The crisscross is 2. I changed the name to wedgeType > * WigglyLineElement is, I assume, copyright 2009, not 2008 Correct > * WigglyLineElement misses JavaDoc on the class and methods. The class > JavaDoc would also be the place to refer to the IUPAC recommendations, > for which @cdk.cite should be used. Added. > * I would like to encourage you to use 80 characters per line According to my Eclipse, all classes are in that range exept the ControllerHub, but this were not my changes. > * please add JavaDoc to methods you have changed API and/or have > rewritten, viz. BasicBondGenerator.generateStereoElement() and the > WedgeLineElement constructors Done. > > > Note the wiggly bond is not yet drawn in the SVG visitor > > (there is a bug report about that). > > Please annotate the class with @cdk.bug accordingly. Done I attach new patches against the repository on pele. > > Egon -- Stefan Kuhn B. Sc. M. A. Software Engineer in the Chemoinformatics and Metabolism Team European Bioinformatics Institute (EBI) Wellcome Trust Genome Campus Hinxton, Cambridge CB10 1SD UK Phone +44 1223 49 2657 Fax +44 (0)1223 494 468 |
From: Stefan K. <ste...@eb...> - 2009-08-06 14:49:13
|
Attached are some more patches relating to the different stereo undefined types. They have to be applied in addition to patches from Tuesday. The mdl readers/writers can now handle bond stereo types 3 and 4, which are both specified in MDL specification. I also added tests for that. Stefan PS: In the future, I will only use the patch tracker for patches. It gets messy on the mailing list... On Tuesday 04 August 2009 10:09:17 Stefan Kuhn wrote: > On Friday 31 July 2009 19:48:00 Egon Willighagen wrote: > > Hi Stefan, > > > > On Fri, Jul 31, 2009 at 7:22 PM, Stefan Kuhn<ste...@eb...> wrote: > > > Please find attached a number of patches. They add a new bond type > > > (wiggly==undefined E/Z) and extend the old undefined stereo to be drawn > > > in both directions, as the other wedges can be. These bond types are > > > IUPAC recommendations. > > > > I have a look at your code, and have these comments: > > > > * why is there need to change the type WedgeLineElement.isDashed? I > > only see use of ==0 and ==1... and if this is really needed, I > > strongly suggest to rename the variable and/or to use an enum. > > The crisscross is 2. I changed the name to wedgeType > > > * WigglyLineElement is, I assume, copyright 2009, not 2008 > > Correct > > > * WigglyLineElement misses JavaDoc on the class and methods. The class > > JavaDoc would also be the place to refer to the IUPAC recommendations, > > for which @cdk.cite should be used. > > Added. > > > * I would like to encourage you to use 80 characters per line > > According to my Eclipse, all classes are in that range exept the > ControllerHub, but this were not my changes. > > > * please add JavaDoc to methods you have changed API and/or have > > rewritten, viz. BasicBondGenerator.generateStereoElement() and the > > WedgeLineElement constructors > > Done. > > > > Note the wiggly bond is not yet drawn in the SVG visitor > > > (there is a bug report about that). > > > > Please annotate the class with @cdk.bug accordingly. > > Done > I attach new patches against the repository on pele. > > > Egon -- Stefan Kuhn B. Sc. M. A. Software Engineer in the Chemoinformatics and Metabolism Team European Bioinformatics Institute (EBI) Wellcome Trust Genome Campus Hinxton, Cambridge CB10 1SD UK Phone +44 1223 49 2657 Fax +44 (0)1223 494 468 |
From: Egon W. <ego...@gm...> - 2009-08-06 14:55:36
|
Stefan, On Thu, Aug 6, 2009 at 4:49 PM, Stefan Kuhn<ste...@eb...> wrote: > Attached are some more patches relating to the different stereo undefined > types. They have to be applied in addition to patches from Tuesday. The mdl > readers/writers can now handle bond stereo types 3 and 4, which are both > specified in MDL specification. I also added tests for that. does the order in which they should be applied matter? Right now, they are almost all called 0001-foo.patch, and I have no clue about the order... I also very much appreciate if you could discuss what each patch is doing, so that I have a clue where what is supposed to go. The above suggests a patch to the MDL reader, which should go up for review for CDK master directly, or cdk-1.2.x... In particular, keeping patches of the same nature together and separate, it makes things easier to review... two patches against CDK master for the MDL reader as much easier to review then a whole set of 8 patches... BTW, I also strongly suggest to use the patch tracker for patches against CDK master and cdk-1.2.x... Egon -- Post-doc @ Uppsala University http://chem-bla-ics.blogspot.com/ |