Thread: [Rdkit-devel] Chem/Draw code: new feature and some refactoring
Open-Source Cheminformatics and Machine Learning
Brought to you by:
glandrum
Menu
▾
▴
rdkit-devel
|
From: <gi...@gm...> - 2011-01-25 11:35:08
|
I've been working lately on adding a feature to the draw code, that is, highlighting atoms by an arbitrary color. This work lead me to some refactoring to the Chem/Draw python code; basically, I added a base canvas class that specialized classes derives to draw on different backends (agg, sping, cairo). I'm sharing it now because the Q4 release was done, so it looks a good timing to eventually drop some new code in the repo and avoid the need to rebase my changes when you make commits to the main SVN repo. So, the code is in my github space here: https://github.com/giallu/rdkit/commits/draw_refactor where you can find the current status and the individual commits; if needed I can send a patch series on this list instead. I'm obviously open to any code review questions and willing to revise my changes in case you want any improvement before considering a merge. Best regards G. -- Gianluca Sforna http://morefedora.blogspot.com http://identi.ca/giallu - http://twitter.com/giallu |
|
From: Greg L. <gre...@gm...> - 2011-01-26 01:28:19
|
Dear Gianluca, On Tue, Jan 25, 2011 at 6:34 AM, gi...@gm... <gi...@gm...> wrote: > I've been working lately on adding a feature to the draw code, that > is, highlighting atoms by an arbitrary color. This work lead me to > some refactoring to the Chem/Draw python code; basically, I added a > base canvas class that specialized classes derives to draw on > different backends (agg, sping, cairo). Thanks for this. I am traveling at the moment and probably won't have time to take a look at it until I'm back in Basel, so I won't be able to comment until the middle of next week. > I'm sharing it now because the Q4 release was done, so it looks a good > timing to eventually drop some new code in the repo and avoid the need > to rebase my changes when you make commits to the main SVN repo. > > So, the code is in my github space here: > https://github.com/giallu/rdkit/commits/draw_refactor > > where you can find the current status and the individual commits; if > needed I can send a patch series on this list instead. > > I'm obviously open to any code review questions and willing to revise > my changes in case you want any improvement before considering a > merge. I will take a look when I'm back in Basel and have a bit more time and then get back to you with comments and suggestions. Thanks again for the contribution, -greg |
|
From: Uwe H. <uw...@fa...> - 2011-02-01 12:16:57
|
Hi, Am Dienstag, den 25.01.2011, 20:27 -0500 schrieb Greg Landrum: > Dear Gianluca, > > On Tue, Jan 25, 2011 at 6:34 AM, gi...@gm... <gi...@gm...> wrote: > > I've been working lately on adding a feature to the draw code, that > > is, highlighting atoms by an arbitrary color. This work lead me to > > some refactoring to the Chem/Draw python code; basically, I added a > > base canvas class that specialized classes derives to draw on > > different backends (agg, sping, cairo). > > Thanks for this. I am traveling at the moment and probably won't have > time to take a look at it until I'm back in Basel, so I won't be able > to comment until the middle of next week. > just some idea: what about removing sping and agg altogether ? The code will be simpler and bug reports also, because it's clear which backend is used. Questions: 1) I don't know if there is a feature/backend which isn't supported by cairo ? 2) Is the cairo quality sufficient (cairo itself , not the actual implementation in rdkit)? 3) speed (when used for many images) 4) ... regards Uwe |
|
From: <gi...@gm...> - 2011-02-01 23:41:21
|
On Tue, Feb 1, 2011 at 12:51 PM, Uwe Hoffmann <uw...@fa...> wrote: > just some idea: what about removing sping and agg altogether ? Well, agg is not really there, you still need to get it, compile the library and let rdklit find it in order to use that backend. On the other hand, sping is embedded in the rdkit repository and I agree it would be better removed from the rdkit tarball. If the patches will get merged, I'd like to make more changes along these lines: 1. remove the sping code 2. make the backend selectable from the API 3. make cairo the default backend 4. further refactor backends (there is still much duplicated code) 5. last, but not least, apply more PEP8 > The code will be simpler and bug reports also, because it's clear > which backend is used. > Questions: > 1) I don't know if there is a feature/backend which isn't supported > by cairo ? For my purposes it's perfectly fine, but of course I have no idea how it's used outside the (few) projects I know of. > 2) Is the cairo quality sufficient (cairo itself , not the actual > implementation in rdkit)? IMHO, they are of good quality > 3) speed (when used for many images) did not run any benchmark, but we can exercise some code and gather numbers. Are you referring just to the draw code or depiction+drawing? Cheers G. -- Gianluca Sforna http://morefedora.blogspot.com http://identi.ca/giallu - http://twitter.com/giallu |
|
From: Greg L. <gre...@gm...> - 2011-02-02 04:24:55
|
On Wed, Feb 2, 2011 at 12:40 AM, gi...@gm... <gi...@gm...> wrote: > On Tue, Feb 1, 2011 at 12:51 PM, Uwe Hoffmann <uw...@fa...> wrote: >> just some idea: what about removing sping and agg altogether ? > > Well, agg is not really there, you still need to get it, compile the > library and let rdklit find it in order to use that backend. On the > other hand, sping is embedded in the rdkit repository and I agree it > would be better removed from the rdkit tarball. I would rather not remove sping (or the possibility to use aggdraw), for reasons I explained in the previous message. > If the patches will get merged, I'd like to make more changes along these lines: > > 1. remove the sping code > 2. make the backend selectable from the API > 3. make cairo the default backend > 4. further refactor backends (there is still much duplicated code) > 5. last, but not least, apply more PEP8 Aside from the first bullet point, I agree with all of these. I am (finally) back in Basel now; I'll take a look at your changes over the next day or so and make comments. Best Regards, -greg |
|
From: Greg L. <gre...@gm...> - 2011-02-02 04:21:44
|
Dear Uwe, On Tue, Feb 1, 2011 at 12:51 PM, Uwe Hoffmann <uw...@fa...> wrote: > > just some idea: what about removing sping and agg altogether ? > The code will be simpler and bug reports also, because it's clear > which backend is used. > Questions: > 1) I don't know if there is a feature/backend which isn't supported > by cairo ? > 2) Is the cairo quality sufficient (cairo itself , not the actual > implementation in rdkit)? > 3) speed (when used for many images) Cairo is a great toolkit for drawing, and the backend you did is perfectly capable. For systems that have cairo installed, it's almost definitely the best choice to use. However, due to its dependency on GTK Cairo is not easy to build and install. For systems that don't have a cairo installation (for example, almost every machine I work on), the aggdraw canvas provides a high-quality alternative for generating raster images. The sping-based canvas is provided as a lowest-common denominator, pure-python, solution to allow images to be generated from the RDKit out-of-the-box (it also has the advantage of being able to generate pdf and svg, which aggdraw cannot). Best Regards, -greg |
|
From: <gi...@gm...> - 2011-02-02 23:34:42
|
On Wed, Feb 2, 2011 at 5:21 AM, Greg Landrum <gre...@gm...> wrote: > Cairo is a great toolkit for drawing, and the backend you did is > perfectly capable. For systems that have cairo installed, it's almost > definitely the best choice to use. However, due to its dependency on > GTK Cairo is not easy to build and install. I think this is not accurate, it's GTK that requires cairo, cairo itself has very few dependencies. There are also more or less "official" cairo builds for windows referenced from http://cairographics.org/download, in case one do not want to mess with compilers. > the aggdraw canvas provides a high-quality alternative for > generating raster images. The sping-based canvas is provided as a > lowest-common denominator, pure-python, solution to allow images to be > generated from the RDKit out-of-the-box (it also has the advantage of > being able to generate pdf and svg, which aggdraw cannot). I have nothing against keeping the various backends; I'm more in favor of removing the rdkit/sping directory, which looks like an external dependency much like agg, cairo or other stuff not bundled with the rdkit tarball. On a side note, I had a brief look at the code and at first glance there is not much there added over PIL so if you want to provide a simple fallback when no other backend is available I'd suggest replacing sping for a plain PIL backend. Obviously, those are just my 0.02 :) Cheers G. -- Gianluca Sforna http://morefedora.blogspot.com http://identi.ca/giallu - http://twitter.com/giallu |
|
From: Greg L. <gre...@gm...> - 2011-02-11 05:50:04
Attachments:
draw.changes.diff
|
Hi Gianluca, I finally managed to go through your proposed refactoring of the drawing code. I like it. It definitely helps clean things up and make the code more flexible. I made some changes and have attached the diffs (if we have to wait for me to figure out git correctly, we'll be waiting a long time). We can do a round or two via git if you like, and then I'll merge the changes back into the main svn. Best Regards, -greg |
|
From: <gi...@gm...> - 2011-03-19 09:41:31
|
Ok Greg, it took a while to come back at this; hopefully we can speed it up from now on On Fri, Feb 11, 2011 at 6:49 AM, Greg Landrum <gre...@gm...> wrote: > I made some changes and have attached the diffs (if we have to wait > for me to figure out git correctly, we'll be waiting a long time). > > We can do a round or two via git if you like, and then I'll merge the > changes back into the main svn. I reviewed the changes and (hopefully) got them applied as intended; there is more work I'd like to see there but it will take more time. I will be sending the complete patch set here with git send-email, in the meanwhile the code was pushed to: https://github.com/giallu/rdkit/tree/draw_refactor -- Gianluca Sforna http://morefedora.blogspot.com http://identi.ca/giallu - http://twitter.com/giallu |
|
From: Greg L. <gre...@gm...> - 2011-03-19 15:19:09
|
Hi Gianluca, On Sat, Mar 19, 2011 at 10:41 AM, gi...@gm... <gi...@gm...> wrote: > Ok Greg, it took a while to come back at this; hopefully we can speed > it up from now on > > On Fri, Feb 11, 2011 at 6:49 AM, Greg Landrum <gre...@gm...> wrote: >> I made some changes and have attached the diffs (if we have to wait >> for me to figure out git correctly, we'll be waiting a long time). >> >> We can do a round or two via git if you like, and then I'll merge the >> changes back into the main svn. > > I reviewed the changes and (hopefully) got them applied as intended; > there is more work I'd like to see there but it will take more time. > > I will be sending the complete patch set here with git send-email, in > the meanwhile the code was pushed to: > https://github.com/giallu/rdkit/tree/draw_refactor Thanks for the diffs. I'll take a look and get them merged into the svn on a branch over the weekend and then we can review the branch. Best, -greg |
|
From: Greg L. <gre...@gm...> - 2011-03-20 05:33:39
|
The changes are merged in here: https://rdkit.svn.sourceforge.net/svnroot/rdkit/branches/DrawChanges_20March2011 I've verified that the current code works for me on the Mac using cairo, aggdraw, and sping. I will be going through it a bit more over the next couple of days, but it would be great if others could also take a look. -greg |
Thanks for helping keep SourceForge clean.
X