New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds SVG reaction depiction #993
Conversation
return rxn | ||
|
||
#source https://commons.wikimedia.org/wiki/File:Tab_plus.svg | ||
# creative commons license (need attribution? Is link enough? Make our own?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the simplicity (and I guess it could be a lot simpler) and the fact that the wikimedia object is "Share alike" licensed we may want to re-do this. I believe that this would count as a re-distribution and the RDKit's BSD license is not the same as CC-BYSA, so it's probably not technically correct to re-use it.
@NadineSchneider : you may also want to take a look at this, in case you have't already. |
xOffset += xdelta*scale | ||
|
||
# add the plus signs | ||
if col < num_reactants - 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should also be putting the plus signs in the products.
Considering Greg's example above I would suggest generating the 2D coords for the whole reaction so that the bond lengths are the same for all molecules instead of scaling them separately which could be quite tricky. |
I've noticed :). The only other option is scaling the fontsize but the single atom renders are very large. I'll try doing them all at the same time and see what happens. Good idea. Brian Kelley
|
I added a test where I scale so that the font sizes are all the same. Looks better, but has some clipping I need to sort. I think the solution of generating all coords in the same frame is the best solution, may require some c++ work. |
I'm pretty sure we want the whole thing in C++ pretty soon anyway, right? |
def _cleanupRXN(rxn): | ||
"""Cleanup reactions so the computed coords are sane for depiction""" | ||
map(_prepareRxnMol, rxn.GetReactants()) | ||
map(_prepareRxnMol, rxn.GetProducts()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(An aside, not important): I'm wondering about style here. Is using map
idiomatic Python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, regardless of whether it is idomatic or not, this doesn't work with python3, where map just returns an iterator. A direct loop or list comprehension is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really have to make the switch I think.
I made a couple of changes. I'm rendering at a higher resolution and downsampling to remove some issues with bonds. Looks like I introduced some clipping errors that I'll fix. This may be the last update before I figure out how to do the proper c++ rendering to get the bond lengths the same. |
An aside: it looks like travis may be having external connectivity issues. Builds keep failing due to failures downloading stuff from sourceforge or, the most recent failure, binstar. |
Ok, I gather you don't like the scaling I'm using ;) I'll turn it off and On Wed, Jul 27, 2016 at 11:06 AM, Greg Landrum notifications@github.com
|
|
||
# render at a higher resolution | ||
actualSize = subImgSize[:] | ||
subImgSize = (600.,600.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
On Wed, Jul 27, 2016 at 11:21 AM, Greg Landrum notifications@github.com
wrote:
In rdkit/Chem/Draw/init.py
#993 (comment):
get the relative sizes of the molecules
- relativeSizes = [1.0] * len(mols)
- num_mols = len(mols)
- blocks = [''] * num_mols
- hdr = ''
- ftr='/svg:svg'
- rect = ''
- _cleanupRXN(rxn)
- xOffset = 0
render at a higher resolution
- actualSize = subImgSize[:]
- subImgSize = (600.,600.)
this shouldn't be here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rdkit/rdkit/pull/993/files/bef16cadaae66d36545aefdd9c8e3ad5df1aa2ce#r72460448,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJbioDL7BmyBSRSIKg9GjQExQDhJC4wLks5qZ3d2gaJpZM4JVe71
.
@NadineSchneider I'll have some time to try and combine your efforts. Perhaps we should chat over email to set up how we want to do this. |
Enables SVG depiction or reactions in ipython
Also fixes mol svg rendering so fonts don't clip.
There are two rendering modes, standard and relative sizes. relative sizes tries to get all the molecules on the same scale. This is currently not the default.
Still probably work that needs to be done, but I think the results are pretty ok.