-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rdkit reb #177
base: trunk
Are you sure you want to change the base?
Conversation
…ons + improved stability to_rdmol - to_rdmol now has an argument presanitize, which iteratively sanitized - improved bonding/atomic charges, until sanitization succeeds. - There are the functions to_image and get_reaction_image.
…n_image. - to_image now works with svg in JupyterLab
Looks like a great feature! Just a couple of overall comments:
|
sanitize: bool = True, | ||
properties: bool = True, | ||
assignChirality: bool = False, | ||
presanitize: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add presanitize
to doc-string with description - might be worth also mentioning that sanitize must also be True
for this to apply
@@ -213,10 +219,11 @@ def plams_to_rd_bonds(bo): | |||
|
|||
if sanitize: | |||
try: | |||
Chem.SanitizeMol(rdmol) | |||
if presanitize: | |||
rdmol = _presanitize(plams_mol, rdmol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have add unit-test cases which use this method in unit_tests/test_molecule_interfaces.TestRDKit
(see e.g. test_to_rdmol_from_rdmol_roundtrip
).
These could take some input molecules that have issues, and check the output is santised as expected.
Do you have some examples for which this santization is useful?
@@ -1290,3 +1297,602 @@ def canonicalize_mol(mol, inplace=False, **kwargs): | |||
ret = mol.copy() | |||
ret.atoms = [at for _, at in sorted(zip(idx_rank, ret.atoms), reverse=True)] | |||
return ret | |||
|
|||
|
|||
@requires_optional_package("rdkit") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated as also requires optional package pillow
, as users may not be using amspython
.
I have also just sorted out the optional dependencies for PLAMS to make them easier to install, so also added the pillow dependency here: https://github.com/SCM-NV/PLAMS/blob/trunk/pyproject.toml#L59
interfaces/molecule/rdkit.py
Outdated
|
||
|
||
@requires_optional_package("rdkit") | ||
def to_image(mol, remove_hydrogens=True, filename=None, format="svg", as_string=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Added type hints)
* ``mol`` -- PLAMS Molecule object | ||
* ``remove_hydrogens`` -- Wether or not to remove the H-atoms from the image | ||
* ``filename`` -- Optional: Name of image file to be created. | ||
* ``format`` -- One of "svg", "png", "eps", "pdf", "jpeg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(renamed to fmt
to avoid clash with python keyword)
# Draw the image | ||
if classes[format.lower()] is None: | ||
# With AMS version of RDKit MolsToGridImage fails for eps (because of paste) | ||
img = Draw.MolToImage(rdmol, size=(200, 100)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to have the size as an option to the function?
interfaces/molecule/rdkit.py
Outdated
return img_text | ||
|
||
|
||
def get_reaction_image_pil(reactants, products, format, width=200, height=100, as_string=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these "private" (_get_reaction_image_pil
) or add to __init__
?
@@ -230,6 +238,38 @@ def test_to_smiles_and_from_smiles_requires_rdkit_package(self, plams_mols, smil | |||
with pytest.raises(MissingOptionalPackageError): | |||
from_smiles(smiles[0]) | |||
|
|||
def test_to_image_and_get_reaction_image_can_generate_img_files(self, xyz_folder): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have converted the attached example to a unit test.
It's not too fancy, just makes sure the image files can be generated, but then they can be visually inspected too.
I added three things:
I added some example molecules and a test script here.
Example.tar.gz