Visualize fixes #1751
Visualize fixes #1751
Conversation
|
In #1771 somebody reported that mucking around with metadata causes some issues - I haven't looked into how this PR interacts with it, but it'd be nice to know. (Note also that in that issue, the behavior was only observed WITHOUT OpenEye.) |
|
nglviewer/ngl#977 has been merged, so we should be able to get the infer bonds issues fixed soon (eg #1771). I've raised nglviewer/nglview#1092 to see what needs to be done for upstream support, and hopefully with the feedback from that I can put together some sort of interim private method hackery! |
|
#1771 seems to be fixed with this PR BTW, because we don't use PDB for |
|
This is ready for review! I think this is a major improvement on the status quo as is, and guaranteeing correct connectivity in visualizations should come in a later PR after I've had a chance to do the supporting work on NGLView upstream. |
|
Sorry for dragging my feet on reviewing this. Unfortunately I think I need to be babied here; since it's hard for us to write unit tests for something that shows up in the browser, I need to manually tinker with the different combinations of inputs and states, verify that the results are expected, and do a rain dance to ensure that it all works the same on future users' machines. If I understand the change correctly, the important umbrella test would be a protein-ligand complex in which the ligand has a double or triple bond(s)? |
|
Ah, I'd planned to take this one, and would prefer to keep it on my plate unless you're currently mid-review. |
|
Go for it |
There was a problem hiding this comment.
This mostly looks good. I haven't checked behavior yet but two (blocking) things that stand out to me are:
- The rdkit import in _viz.py (see comment)
- The failing CI seems to involve code added by this PR
and this one isn't blocking, but is there a way to query the widget object to see whether bond orders were set? It's fine if not, I'd just love for this to have an automated test :-)
| self.topology.to_file(f, file_format=self.ext) | ||
| structure_string = f.getvalue() | ||
| if RDKIT_AVAILABLE: | ||
| from rdkit.Chem.rdmolfiles import PDBWriter # type: ignore |
There was a problem hiding this comment.
(blocking) Direct cheminformatics toolkit imports outside of toolkit wrappers have caused all sorts of havoc before. Even with the RDKIT_AVAILABLE guard I'm worried about this. Can this be refactored to put the RDKit import and associated code in RDKitToolkitWrapper?
| conformations, include bonds not present in the topology. Rendering | ||
| multiple bonds additionally requires RDKit. |
There was a problem hiding this comment.
(not blocking) Is this the correct interpretation, or am I misunderstanding the intent here?
| conformations, include bonds not present in the topology. Rendering | |
| multiple bonds additionally requires RDKit. | |
| conformations, include bonds not present in the topology. Rendering | |
| bond orders correctly additionally requires RDKit. |
This finishes some stuff left over from the visualization updates - the overloads for
Molecule.visualize()have been corrected, and the wholeensure_correct_connectivitything inTopology.visualize()has been removed.ensure_correct_connectivitywas supposed to do two things:Over in
Molecule.visualize()land, we currently attempt to talk to NGLView with MOL2 and then failover into PDB when RDKit spits the dummy. But RDKit's PDB writer supports writing multiple bonds in CONECT records... so double bonds show up. So I wrote a Topology PDB writer with RDKit... and it works. So that's (2) handled! Not sure if we like this, hate this, or really like it and want to use it for general topology PDB exports. This means visualizing multiple bonds works only with RDKit, and we fall back to OpenMM if RDKit is missing.NGLView supplements CONECT records with bonds inferred from positions, so (1) isn't currently possible for Molecules or Topologies. I've updated the docstrings to tell users about this. This may be fixable soon: nglviewer/ngl#977. If that stalls, we might want to drop the PDB failover and switch to SDF. The catch of SDF is that protein secondary structure is absent, and so is atom metadata like atom names, residue names, etc. These all show up in PDB and MOL2.
So at present there's no way to ensure the correct connectivity, and in the future the most likely path to making it possible won't have a downside. Thus, I removed the option.
# doctest: +SKIPdoesn't render in API docs