Skip to content
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

Fixes #5017 #5103

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Fixes #5017 #5103

merged 1 commit into from
Mar 17, 2022

Conversation

ptosco
Copy link
Contributor

@ptosco ptosco commented Mar 17, 2022

Reference Issue

This PR fixes #5017

What does this implement/fix? Explain your changes.

patchPandasrepr() patches pd.io.formats.html.HTMLFormatter._write_cell(), then it restores the original function before returning. However, while doing so, it also sets an _rdkitpatched attribute on pandas.io.formats.html.HTMLFormatter.
The next time patchPandasrepr() is called, the patch is not applied anymore due to the present of the _rdkitpatched attribute, even though the patch has actually been reverted.
This explains why the DataFrame displays molecules as PNG images only the first time.
The fix is removing the usage of _rdkitpatched attribute entirely as it is not necessary, since the patch is applied and then reverted. Instead, a try..finally block is used to make sure that the patch is reverted before returning even if an exception is raised.

After this PR, running the test described in #5017 yields the desired behavior:

from rdkit import Chem
from IPython.display import HTML
from rdkit.Chem import PandasTools
import pandas as pd

import rdkit
rdkit.__version__
'2022.03.1pre'

df = pd.DataFrame(dict(a=[1], s=['CCN']))
PandasTools.AddMoleculeColumnToFrame(df, smilesCol='s', molCol='m')
df

image

df

image

PandasTools.RenderImagesInAllDataFrames(False)
df

image

df

image

PandasTools.RenderImagesInAllDataFrames(True)
df

image

df

image

@ptosco
Copy link
Contributor Author

ptosco commented Mar 17, 2022

The macOS_x64_java failure was caused by Avalon failing to download from SourceForge, nothing to do with this PR.

Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@greglandrum greglandrum merged commit 2d7eb89 into rdkit:master Mar 17, 2022
@ptosco ptosco deleted the github5017 branch March 17, 2022 13:08
@greglandrum
Copy link
Member

@ptosco : I merged this without actually trying it, and it doesn't seem to actually work in jupyter:
image
I've tried it in both jupyter notebook and jupyterlab (v3.4.4) and get the same behavior in both of them

Which version/OS did you test this on?

@greglandrum
Copy link
Member

@ptosco : I merged this without actually trying it, and it doesn't seem to actually work in jupyter:

The same problem occurs with pandas 1.4.2

@ptosco
Copy link
Contributor Author

ptosco commented Jun 21, 2022

@greglandrum The fix works, the problem is that I inadvertently reverted it in PR #5132:
https://github.com/rdkit/rdkit/pull/5132/files#diff-4d3e0e17d686a9de85fb42b78a37f8fe69e95258f4e6be45899873fa292a2090L200
because I did not merge #5103 into my rdkit-structure-renderer branch before submitting #5132.
Sorry for the inconvenience, I have just submitted #5381 to restore the #5103 fix.

@greglandrum
Copy link
Member

@ptosco : sorry, I should have caught that in the review... the PandasTools code is a real brain bender though.

It would be super shiny if we could figure out some way to actually automatically test this stuff, but I don't see how that would work.

greglandrum pushed a commit that referenced this pull request Jun 22, 2022
Co-authored-by: Tosco, Paolo <paolo.tosco@novartis.com>
greglandrum pushed a commit that referenced this pull request Jul 5, 2022
Co-authored-by: Tosco, Paolo <paolo.tosco@novartis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mol images in DataFrames are drawn only once in Jupyter Lab
2 participants