Draw hydrogen bonds. by rwest · Pull Request #1961 · ReactionMechanismGenerator/RMG-Py

rwest · 2020-05-24T03:26:13Z

Motivation or Problem

Hydrogen atoms were not drawn explicitly, and (therefore) hydrogen bonds were not drawn. Structures held together (or apart) with hydrogen bonds would just collapse on top of themselves. See before/after example below.

Description of Changes

Don't remove an H if it has an H bond.
Draw H-bonds as dotted lines.
Fix a problem that occurred with a complicated system of fused rings.

Testing

from rmgpy.molecule import Molecule
example =  Molecule().from_adjacency_list("""
example
1  O u0 p3 c-1 {2,S} {10,H}
2  N u0 p0 c+1 {1,S} {3,D} {4,S}
3  O u0 p2 c0 {2,D}
4  O u0 p2 c0 {2,S} {7,S}
5  N u0 p1 c0 {6,S} {8,S} {9,S} {7,H}
6  O u0 p2 c0 {5,S} {10,S}
7  H u0 p0 c0 {4,S} {5,H}
8  H u0 p0 c0 {5,S}
9  H u0 p0 c0 {5,S}
10 H u0 p0 c0 {6,S} {1,H}
""")
example
display(example)
for m in example.generate_h_bonded_structures():
    display(m)

Before:

After:

codecov · 2020-05-27T20:00:38Z

Codecov Report

Merging #1961 into master will increase coverage by 0.67%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master    #1961      +/-   ##
==========================================
+ Coverage   46.59%   47.26%   +0.67%     
==========================================
  Files          88       88              
  Lines       23118    23141      +23     
  Branches     6005     6013       +8     
==========================================
+ Hits        10771    10937     +166     
+ Misses      11197    11056     -141     
+ Partials     1150     1148       -2

Impacted Files	Coverage Δ
rmgpy/molecule/draw.py	`53.52% <53.84%> (+13.46%)`	⬆️
arkane/kinetics.py	`12.24% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b040978...90b5190. Read the comment docs.

rwest · 2020-06-10T18:07:45Z

All checks pass! 💯

alongd

Looking good! I added some questions, please take a look

alongd · 2020-06-10T23:14:27Z

                        if cycle is None or len(cycle0) > len(cycle): cycle = cycle0
            cycle0 = cycle1
+            if cycle is None:
+                break


could you explain this addition?

Without it trying to draw

1 O u0 p2 c0 {5,S} {8,S} 2 O u0 p2 c0 {6,S} {7,S} 3 O u0 p3 c-1 {6,S} {8,H} 4 O u0 p2 c0 {6,D} {9,H} 5 N u0 p1 c0 {1,S} {7,H} {9,S} {10,S} 6 N u0 p0 c+1 {2,S} {3,S} {4,D} 7 H u0 p0 c0 {2,S} {5,H} 8 H u0 p0 c0 {1,S} {3,H} 9 H u0 p0 c0 {4,H} {5,S} 10 H u0 p0 c0 {5,S}

leads to

Traceback (most recent call last): File "/Users/rwest/Code/RMG-Py/rmgpy/molecule/draw.py", line 209, in draw self._generate_coordinates() File "/Users/rwest/Code/RMG-Py/rmgpy/molecule/draw.py", line 366, in _generate_coordinates self._generate_ring_system_coordinates(backbone) File "/Users/rwest/Code/RMG-Py/rmgpy/molecule/draw.py", line 564, in _generate_ring_system_coordinates atoms.remove(cycle) ValueError: list.remove(x): x not in list

If you replace the break with a continue then you enter and endless loop (due to the while)

With the break we get the image

...which is not great - the H is being drawn exactly on top of an O which hides it - but at least it doesn't crash.

alongd · 2020-06-10T23:16:42Z

        (`x2`,`y2`), and update the bounding rectangle if necessary.
+
+        For a dashed line, `dashed` be a list of on/off segment lengths,
+        or `True` in which case [3.5,3.5] is used.


Minor: please use double "`" marks for True, a single mark on each side represents a link in rst (double, a code)

alongd · 2020-06-10T23:18:41Z

        if dashed:
-            cr.set_dash([3.5])
+            if dashed is True:
+                dashed = [3.5]


The docsting says [3.5, 3.5], here it's [3.5], which is correct?
Could you also provide a general explanation for why dash is sometimes a bool and sometimes a list? I understand it wasn't introduced by this PR.

Other places in the code were already calling this method with dashed=True and get a default dash size of 3.5. I wanted a way to specify the dash size. I guess it could be done with an extra optional argument dash_size instead of overloading this one if you think that's better.

([3.5] is equivalent to [3.5, 3.5] when specifying dash sizes in cairo, but to avoid people needing to know that I've changed it to [3.5, 3.5] in a fixup commit)

If this arg is currently being used only as a bool, let's make a new one, unless you think it's very redundant

This better? c695be3

yes, thanks!

alongd · 2020-06-10T23:22:05Z

+        """
+        )
+        try:
+            from cairocffi import PDFSurface


Are cairocffi and cairo OS-dependent? If not, can we assume one of them should be present?

I don't know - I just copied boilerplate from all the other drawing tests. Changing them all is out of scope of this PR.

alongd · 2020-06-10T23:23:14Z

            os.unlink(path)
        polycycle = Molecule(smiles="C123CC4CC1COCC2CCC34")
-        surface, cr, (xoff, yoff, width, height) = self.drawer.draw(self.molecule, file_format='pdf', target=path)
+        surface, cr, (xoff, yoff, width, height) = self.drawer.draw(polycycle, file_format='pdf', target=path)


rwest · 2020-06-11T04:44:51Z

I addressed your suggestions with fixup commits (so you can review them independently) but they should be squashed on rebasing to merge.

rwest · 2020-06-11T05:34:38Z

I added another commit which pushes atoms apart if they would have otherwise been on top of each other due to the failings of the algorithm for fused multicycles.

Now the image in the PR description up top looks like this:

at first glance it's not as neat and pretty as before, but it's actually less misleading. Now that the atoms aren't all piled on top of each other at the origin, you can at least count and see them. These pictures all have 4 O atoms showing!

alongd · 2020-06-11T10:48:14Z

@rwest, please do what you think is right re ading dash_size (I added a minor comment), then feel free to squash

1. Don't remove an H if it has an H bond. 2. Draw H-bonds as dotted lines. 3. Fix a problem that occurred with a complicated system of fused rings, if you have two rings that share more than two atoms, then we can't draw it well. Never have been able to (that's why we punted this task to RDKit whenever possible). But after this fix, at least we don't crash.

Just checks we don't crash - doesn't check they look good. But should increase the code coverage at least.

A copy/paste slip meant it wasn't actually drawing the polycyclic molecule. (Note that this test just ensures it doesn't crash, it doesn't see how it looks, else someone might have noticed it was drowing the wrong thing)

The codacy code quality tests were failing because pylint detected unused variables. Prefixing them with an underscore (or with 'dummy_') avoids this test.

Sometimes when drawing fused ring systems where the rings share more than 2 atoms, the algorithm can't cope and doesn't place some ring systems. Those "unplaced" atoms all end up piled on top of each other at the origin. I don't know how to fix that fused ring system problem (usually we get RDKit to do it, which works fine) so this change detects atoms that are on top of each other and pushes them apart a bit. They may not be pushed in the best direction, but at least they won't get drawn on top of each other, so you can see the atoms in the drawing.

rwest · 2020-06-12T13:13:08Z

OK, all fixups rebased, tests passing again. Should I merge, or wait for someone else to?

alongd

Thanks!

This was added in PR #1961 which indroduced the ability to draw hydrogen bonds. It was removed without explanation in ec95153 which made drawing work for fragments. That broke drawing of hydrogen bonds. The unit test for drawing hydrogen bonds exists, but just checks that a PDF is made, not what it looks like. So this wasn't detected.

Pull request #1961 introduced the ability to draw hydrogen bonds. To do so, Hydrogen atoms that have H bonds shouldn't be removed from the drawings. This check was removed without explanation in ec95153 which was designed to make drawing work for fragments. That broke drawing of hydrogen bonds. This pull request fixes it, and also removes some code duplication.

This was added in PR #1961 which indroduced the ability to draw hydrogen bonds. It was removed without explanation in ec95153 which made drawing work for fragments. That broke drawing of hydrogen bonds. The unit test for drawing hydrogen bonds exists, but just checks that a PDF is made, not what it looks like. So this wasn't detected.

rwest added Complexity: Low Status: Ready for Review PR is complete and ready to be reviewed labels May 24, 2020

rwest commented May 27, 2020

View reviewed changes

Comment thread rmgpy/molecule/draw.py Outdated

rwest force-pushed the drawHbonds branch from 2ecc373 to b6655f8 Compare May 27, 2020 19:31

rwest force-pushed the drawHbonds branch from b6655f8 to e00e17b Compare June 10, 2020 13:38

rwest requested a review from alongd June 10, 2020 20:11

alongd reviewed Jun 10, 2020

View reviewed changes

rwest force-pushed the drawHbonds branch from 9c6a8dc to a6db718 Compare June 11, 2020 13:51

rwest added 4 commits June 11, 2020 21:45

Added a unit test for drawing Hydrogen bonds

a3888e5

Just checks we don't crash - doesn't check they look good. But should increase the code coverage at least.

Prefix unused variables with _ to satisfy pylint

e2b590f

The codacy code quality tests were failing because pylint detected unused variables. Prefixing them with an underscore (or with 'dummy_') avoids this test.

rwest force-pushed the drawHbonds branch from a6db718 to 17e7c00 Compare June 12, 2020 01:47

rwest force-pushed the drawHbonds branch from 17e7c00 to 90b5190 Compare June 12, 2020 02:38

alongd approved these changes Jun 12, 2020

View reviewed changes

alongd merged commit 17c2d43 into master Jun 12, 2020

alongd deleted the drawHbonds branch June 12, 2020 13:55

rwest mentioned this pull request Nov 20, 2024

Draw Hydrogen bonds (again) #2734

Merged

Uh oh!

Conversation

rwest commented May 24, 2020

Motivation or Problem

Description of Changes

Testing

Uh oh!

Uh oh!

codecov Bot commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rwest commented Jun 10, 2020

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alongd Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwest commented Jun 11, 2020

Uh oh!

rwest commented Jun 11, 2020

Uh oh!

alongd commented Jun 11, 2020

Uh oh!

rwest commented Jun 12, 2020

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 27, 2020 •

edited

Loading

alongd Jun 10, 2020 •

edited

Loading