Wrap RDKit drawing code for AtomGroups #2900
Conversation
|
Hello @cbouy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-08-31 16:50:33 UTC |
|
I added a metaclass and a base class for "formatters" (i.e. classes that can modify the representation of other objects in interactive shells/notebooks), in case we want to add another visualization package like nglview to display atomgroups directly. Also, I'm not sure how to test the AtomGroup representation and the generation of images... |
Codecov Report
@@ Coverage Diff @@
## develop #2900 +/- ##
===========================================
+ Coverage 87.20% 92.57% +5.37%
===========================================
Files 167 189 +22
Lines 21744 24690 +2946
Branches 3186 3196 +10
===========================================
+ Hits 18961 22857 +3896
+ Misses 2258 1787 -471
+ Partials 525 46 -479
Continue to review full report at Codecov.
|
|
I'm moving this to the 2.1.0 milestone |
|
Maybe this helps with the SVG testing? https://github.com/ldomic/mdanalysis/blob/2af2d99447cc06d96fd3695b400b9f3bcbb0f4d5/testsuite/MDAnalysisTests/visualization/test_lintools.py |
| Parameters | ||
| ---------- | ||
| size : tuple | ||
| default width and height of images, in pixels |
There was a problem hiding this comment.
Would it be possible to set up your own width and height for an image? If so, then default is not necessary - the fact that it is set up as an attribute of the function suggests the default :)
| format = "RDKIT" | ||
|
|
||
| def __init__(self, size=(450, 250), max_atoms=200, removeHs=True, | ||
| kekulize=True, drawOptions=None, useSVG=False): |
There was a problem hiding this comment.
I am of course biased, but if SVG were to be True by default (or even as the only option), it would become much easier to test and then we could leave it to the users to convert the image in the format of their choosing - PNG, JPEG etc - or if something like this is needed, to have a separate function that converts SVG to PNG or JPEG - it seems like there are some libraries that could do this easily. That way the testing burden is reduced, as you know that RDKitDrawer produces expected content and only the conversion remains untested,
| size : tuple | ||
| default width and height of images, in pixels | ||
| max_atoms : int | ||
| AtomGroups with more atoms that this limit won't be displayed as |
There was a problem hiding this comment.
More of a question - I reckon this would be enough to show a regular lipid molecule without the hydrogens? With increased interest in lipids in MD, it could be useful to consider their drawings as well.
| keep_3D : bool | ||
| Keep or remove 3D coordinates to generate the image | ||
| """ | ||
| mol = ag.convert_to("RDKIT") |
There was a problem hiding this comment.
One of the tricks I found while trying to make this happen a few years ago was that RDKit drew the molecules best when they were loaded as SMILES - I suppose they might already be, but if not it is worth a shot!
| if isinstance(fp, BytesIO): | ||
| b64 = b64encode(fp.getvalue()).decode("ascii") | ||
| display(HTML(f"<img src='data:image/gif;base64,{b64}' />")) | ||
|
|
There was a problem hiding this comment.
| else: | |
| raise ValueError("Creation of GIF requires a trajectory as part of the Universe") |
Or something along those lines
| class _Formattermeta(type): | ||
| """Automatic Formatter registration metaclass | ||
|
|
||
| .. versionadded:: 2.0.0 |
There was a problem hiding this comment.
I guess these need to move up to 2.1.0 (provisionally?)
| shell = get_ipython() | ||
| except NameError: | ||
| shell = None | ||
| warnings.warn("You should be in an interactive python shell (IPython or " |
There was a problem hiding this comment.
Could I not create visualizations outside of IPython ecosystem? If e.g. I would like to create a pipeline such as LINTools used to be, I might struggle with this requirement, as it requires chaining functions in this PR and new ones. I do wonder if this requirement could make the visualization packages harder to use..
|
|
||
| def test_ag_to_img_svg(self, u, drawer): | ||
| svg = drawer.atomgroup_to_image(u.atoms, useSVG=True) | ||
| assert len(svg) == 1275 |
There was a problem hiding this comment.
I think this could be extended to check the contents of the file as well as the length :)
Part of #2468
Depends on #2775
Changes made in this Pull Request:
from MDAnalysis.visualization.RDKit import RDKitDrawer, the default representation of the AtomGroup is changed for small molecules: it displays an image of the AtomGroup using RDKit's drawing code (and the default representation for large AtomGroups)drawer.atomgroup_to_image(ag, ...). There is also some code to automatically display or save gifs from an atomgroup:PR Checklist
Tagging @MDAnalysis/coredevs
I'm not sure if I've put the code in the right place but "visualization" made sense to me