Improve visualization with RDKit backend

[Yoshanuikabundi]

Previously, the Molecule.visualize() method with the rdkit backend was a little ugly. It rendered in low-resolution PNG and would use the conformer coordinates to position atoms. This caused results that were inconsistent depending on whether conformers existed in the Molecule, and at worst could obscure complex molecules. Hydrogens were also always rendered explicitly, which is at least unconventional.

This PR adds an explicit_hydrogens argument to the Molecule.visualize() method that determines whether hydrogens should be rendered, forces a recalculation of 2D coordinates for visualization with every call, and draws in SVG format so that lines are crisp and clear at any scale. It also allows the rdkit backend to accept the existing width and height arguments. Finally, it prefers the CoordGen library for 2d coordinate generation - it seems to provide nicer coordinates, especially with explicit hydrogens.

I've had to screenshot the SVGs to get GitHub to like them, but you can see even from the screenshots how much crisper they are. On the notebook, they render as SVGs.

Before:

Before, with a conformer:

After, with implicit hydrogens (the new default):

After, with explicit hydrogens:

Closes #759, at least for the rdkit backend.

• Tag issue being addressed
• Add tests
• Update docstrings/documentation, if applicable
• Lint codebase
• Update changelog

[codecov]

Codecov Report

Merging #1001 (a5a4a53) into master (f5b5558) will decrease coverage by 10.03%.
The diff coverage is 87.50%.

❗ Current head a5a4a53 differs from pull request most recent head 7a7dd10. Consider uploading reports for the commit 7a7dd10 to get more accurate results

[j-wags]

This looks really cool, and I'm glad we're closing that old issue. Based on our conversation in today's meeting, could you additionally:

• Rename the kwarg to show_hydrogens
• Make the default be to show_hydrogens=True
• Implement this behavior as well for the OE and NGLView backends

Also, I think @mattwthompson is more familiar with the drawing code than me, so he may be a better reviewer (once you're ready for someone to take a look!)

[adalke]

The drawing code looks fine to me.

[j-wags]

A clearer example of why this change is needed:

Using the current codebase

from openff.toolkit.topology import Molecule
mol = Molecule.from_smiles('c1ccccc1COCc2ccccc2')
mol.generate_conformers()
mol.visualize(backend='rdkit')

[mattwthompson]

I'm far from an expert in these features (when I worked on #560 etc. I was pretty much learning it at the same time) but I'd be happy to review once this is ready. (It might be now?)

[j-wags]

Thanks @mattwthompson. I'm going to take over finishing this so that @Yoshanuikabundi can move on to learning Interchange in a week or two. I'll still need to thread the show_hydrogens kwarg through to OETKW, but I will ping you for a review once that's done :-)

[Yoshanuikabundi]

@j-wags I've implemented the changes we discussed. I think this is ready for review! @mattwthompson

• I haven't implemented show_hydrogens for nglview, because it would basically involving rewriting the code that nglview uses to construct the default representations. But I can!
• I renamed show_hydrogens to show_all_hydrogens, because both toolkits sometimes label terminal groups as "NH2" or whatever. show_all_hydrogens=True means that every hydrogen is in the picture, but False doesn't mean that none of them are
• I've made using ignored arguments an error (eg, show_all_hydrogens with the "nglview" backend). This is technically a breaking change, but Jeff I think you mentioned it in our call.
• I've added the ignored arguments error behaviour to the tests, but not anything else - I'm not sure how to test visualization.
• I've fixed a major bug in Molecule.to_rdkit(), where rdkit could add implicit hydrogens to molecules created from the toolkit. This was particularly pathological with radicals, which lost their radicalness on export. I've written up this change in commit a0263f9. The fix is one line, but it means that rdkit will not try to guess about the existence of any hydrogens not included explicitly.

[mattwthompson]

Blocking: This seems like something that could potentially have downstream side effects since this code path is not restricted to visualization. I have a certain amount of confidence seeing that the tests still pass, but I'd like either @j-wags's blessing or another explanation as to why this won't accidentally break things. Maybe that's as simple as "well, we wouldn't want the result of .to_rdkit()` to have explicit hydrogens anyway" ?

[Yoshanuikabundi]

I think I'd like @j-wags blessing too, but I give that explanation in the commit message to a0263f9. In short, we are already guaranteed to explicitly represent all our hydrogens, so its important to tell rdkit not to infer the existence of any other hydrogens. Without this change, converting a radical species to rdkit changes the molecule's identity. There are probably other related undiscovered bugs too.

[mattwthompson]

Thanks for the detailed explanation in that commit. I'm on board with this solution given your explanation, the fact that the tests are passing, our general lack of support/interest in radical species, and the likelihood that there are bigger problems with handling radicals. Until they're a key part of our force fields, I don't think it's worth losing sleep over them.

Given that Jeff is somewhat priority paralyzed at the moment I think it would be best to merge this in its current state. Unlikely as it may be, we could still catch breaking behavior before a next release. But please add a note to the "Behavior changed" section summarizing this change.

[j-wags]

Thanks for looping me in. I think this is a good call -- On the surface this is exactly what we want (OFFMols ALWAYS have all atoms explicit), and I can't think of any cases where this would have unintended effects. So this looks good to me, thanks for catching it @Yoshanuikabundi!

[mattwthompson]

I think this is good to go once the single blocking comment is resolved. I did make some changes myself; feel free to poke through the "resolved" conversations if anything is disagreeable.

Below is the notebook I used while reviewing; everything looked good on this branch (including expected failures) and a couple of the RDKit cells looked weird using upstream/master, weird in similar ways to #759. I think that issue can be closed if this is merged; or at least I didn't see obvious things wrong with OpenEye-based visualization.
1001-review.ipynb.txt

[j-wags]

Looks great to me. Thanks @Yoshanuikabundi!