2ddrawenhancements2931

[DavidACosgrove]

Completes everything in #2931, AFAIK.

What does this implement/fix? Explain your changes.

As well as #2931, shows radicals in drawing.
Some general tidying in relevant code.

Any other comments?

[DavidACosgrove]

These were requests from MedChemica, part of what they paid for. The first 2 were because they wanted the option of the drawing not autoscaling up. Their example was Clc1ccccc1 which gets very large on a normal-sized canvas. But they did want it to scale down if it wouldn't fit at the standard scale. The intention is that they'll pick a scale whereby, for example, most benzene rings are drawn the same size regardless of how much space is occupied in the canvas. The rotation was so that the user could specify a preferred orientation, such as by being offered a set of different rotations, that could then be applied to all renditions of that molecule. They have an option in their software for preparing a powerpoint slide with multiple copies of a molecule with different annotations and they wanted to be able to have them all as the user likes.

…

On Tue, Mar 3, 2020 at 3:34 PM Greg Landrum ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Code/GraphMol/MolDraw2D/MolDraw2D.h <#2979 (comment)>: > @@ -126,6 +130,19 @@ struct RDKIT_MOLDRAW2D_EXPORT MolDrawOptions { ColourPalette atomColourPalette; // the palette used to assign // colors to atoms based on // atomic number. -1 is the default value + double fixedScale; // fixes scale to this fraction of draw window width, so + // an average bond is this fraction of the width. If + // scale comes out smaller than this, reduces scale, but + // won't make it larger. Default -1.0 means no fix. + double fixedBondLength; // fixes the bond length (and hence the scale) to + // always be this number of pixels. Assuming a bond + // length in coordinates is 1, as is normal. If + // scale comes out smaller than this, reduces scale, but + // won't make it larger. Default -1.0 means no fix. + // If both fixedScale and fixedBondLength are > 0.0, + // fixedScale wins. + double rotate; // angle in degrees to rotate coords by about centre before what's the use case for this one? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2979?email_source=notifications&email_token=ACGF2FSYYK5THF27L4Z2JFTRFUPQRA5CNFSM4LAIGFOKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXYHW7Y#pullrequestreview-368081791>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGF2FWPHPKTO7UHIT7USRDRFUPQRANCNFSM4LAIGFOA> .

-- David Cosgrove Freelance computational chemistry and chemoinformatics developer http://cozchemix.co.uk

[d-b-w]

Since this is about depiction, it might be helpful to include some before/after images of depictions in the PR.

[DavidACosgrove]

These two pictures give some of the main differences. Top is the new, bottom is the old.

[greglandrum]

I'm not going to pretend that I read every line, but the images look a lot better than they did before and what I did read through seems solid.
Some suggestions anyway, of course. :-)

[greglandrum]

I know it's technically correct, but I find repeating variable names like this confusing. How about:

Suggested change

	Point2D lc = getDrawCoords(cds[i]);
	auto lci = getDrawCoords(cds[i]);

[greglandrum]

Is this really supposed to be string_height? Seems odd to adjust the x coordinate by the height.

[greglandrum]

Why the change of name for this file?

[greglandrum]

Suggested change

	for(double plusLoc: plusLocs) {
	for(auto plusLoc: plusLocs) {

[greglandrum]

this set of if-elses could be a switch.

[greglandrum]

could also be a switch

[greglandrum]

the code below will misbehave if either xradius or yradius are 0. Maybe worth testing for that here?

[greglandrum]

You could do the transformation here using this class and avoid duplicating some code:
https://github.com/rdkit/rdkit/blob/master/Code/Geometry/Transform2D.h#L21

[greglandrum]

Looking through the output of the tests from the current version (c70a3f7), here are some observations.

This is all going to be pointing out problems (or things I perceive as problems), so I should point out at the very beginning that this looks a lot better than what the RDKit currently does. A big step forward!

Now on to the complaining. :-)

This is rxn_test1_1.svg in Chrome:

O:3 in the reactants is placed a bit oddly(not terrible), the H on N:6 in the products is cut off, and the H2O:4 is too close to the plus sign in the products.

rxn_test1_5.png:

The radicals here (and the other PNGs) are mighty small. Not sure if this is fixable, but the placement of the radicals on C:2 and C:7 is confusing/difficult to see

rxn_test2_1.png

The second reactant is mightly close to the "+" sign. May not be trivial to fix.

rxn_test2_2_3.png:

I don't like the H appearing centered over "N:4". Seems like it should be over the N

test4_1.png:

The boxes showing the atom collisions are not closed

test5_2.png:

the line width for the attachment point is too narrow. It's fine (maybe too thick, but having it be the same as the bonds is not a bad starting point) in the corresponding SVG

test6_1.svg:

This is another H centering thing. I think the "H" should be centered under the "N"

I didn't really see anything else new and "disturbing".

[DavidACosgrove]

Hi Greg, Many thanks for the comments. I hadn't looked at the reactions yet, other than rxn_test1_1.svg. THe H2O:4 in that case is too far left because it is centred on the O:4 atom and the H2 drawn "back" from that. It's the same rule for the placement of H2N. It felt like a relatively rare special case (when it's a one-heavy-atom product) so I left it in the first pass. It might fix itself when I look at things like rxn_tet2_1.png where the reactant spacing needs attention. Most of the other things you highlight are manifestations of the same thing, shown most clearly by O:4H in rxn_test1_1.svg. The atomic symbol and the atom mapping part are treated as a single entity when placed, so that, in this example, the single bond is below the colon rather than the O. The double bond to the O:4 in rxn_test1_5.png makes it look particularly bad in that case. The H2+ in test6_1.svg has the H on the atom's x-coord but because the N has isotopic information, its symbol has been pushed to one side to accommodate the 15. I see another horrible session of whackamole ahead in trying to fix these whilst not breaking anything else. The size of the radicals in the PNG is cleaerly sub-optimal. If there's an easy way of embedding a unicode character in a C++ string, that can be solved like that, probably. There is clearly a bug in the placement of them on C atoms wrt N. The other pieces are trivial, I think. Cheers, Dave

…

On Fri, Mar 6, 2020 at 7:01 AM Greg Landrum ***@***.***> wrote: Looking through the output of the tests from the current version (c70a3f7 <c70a3f7>), here are some observations. This is all going to be pointing out problems (or things I perceive as problems), so I should point out at the very beginning that this looks a lot better than what the RDKit currently does. A big step forward! Now on to the complaining. :-) This is rxn_test1_1.svg in Chrome: [image: image] <https://user-images.githubusercontent.com/540511/76055883-35fff280-5f75-11ea-98eb-beeae7a4cb3a.png> O:3 in the reactants is placed a bit oddly(not terrible), the H on N:6 in the products is cut off, and the H2O:4 is too close to the plus sign in the products. rxn_test1_5.png: [image: image] <https://user-images.githubusercontent.com/540511/76058900-d5c17e80-5f7d-11ea-8fc1-777be28cbe1c.png> The radicals here (and the other PNGs) are mighty small. Not sure if this is fixable, but the placement of the radicals on C:2 and C:7 is confusing/difficult to see rxn_test2_1.png [image: image] <https://user-images.githubusercontent.com/540511/76058996-0ef9ee80-5f7e-11ea-9650-5d2711dac2aa.png> The second reactant is mightly close to the "+" sign. May not be trivial to fix. rxn_test2_2_3.png: [image: image] <https://user-images.githubusercontent.com/540511/76059096-49638b80-5f7e-11ea-8aa9-421d2fc283c5.png> I don't like the H appearing centered over "N:4". Seems like it should be over the N test4_1.png: [image: image] <https://user-images.githubusercontent.com/540511/76059285-bb3bd500-5f7e-11ea-9a5a-cf9f1b62b0f5.png> The boxes showing the atom collisions are not closed test5_2.png: [image: image] <https://user-images.githubusercontent.com/540511/76059340-e0304800-5f7e-11ea-8e3e-c9ab795508e5.png> the line width for the attachment point is too narrow. It's fine (maybe too thick, but having it be the same as the bonds is not a bad starting point) in the corresponding SVG test6_1.svg: [image: image] <https://user-images.githubusercontent.com/540511/76059427-1a99e500-5f7f-11ea-939a-69a33dd068ba.png> This is another H centering thing. I think the "H" should be centered under the "N" I didn't really see anything else new and "disturbing". — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2979?email_source=notifications&email_token=ACGF2FV54W22VG4ZFS7RQCDRGCNWHA5CNFSM4LAIGFOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOAJPHA#issuecomment-595629980>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGF2FQOYZN3WLZNKOD2MEDRGCNWHANCNFSM4LAIGFOA> .

-- David Cosgrove Freelance computational chemistry and chemoinformatics developer http://cozchemix.co.uk