pass max_mols to Draw.MolsToGridImage #222
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
==========================================
+ Coverage 91.99% 92.33% +0.34%
==========================================
Files 46 46
Lines 3873 3863 -10
==========================================
+ Hits 3563 3567 +4
+ Misses 310 296 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
datamol/viz/_viz.py
Outdated
| in_notebook = get_ipython() is not None | ||
|
|
||
| if in_notebook: | ||
| _kwargs["maxMols"] = max_mols |
There was a problem hiding this comment.
I wonder whether we should actually always pass max_mols to maxMols in Draw.MolsToGridImage below? What do you think?
There was a problem hiding this comment.
hum actually I am not sure... what is your use case here?
On one side, I would say, the user will expect something like max_mols to be identical to the upstream maxMols. But on the other side, datamol is doing some preprocessing which where we "cut" to max_mols earlier.
Whatever our final decision, it should be clearly documented in the docstring!
There was a problem hiding this comment.
My first commit here always passed max_mols to maxMols in Draw.MolsToGridImage, but all tests failed. It turns out this arg is only valid when running in an IPython context.
There was a problem hiding this comment.
There was a problem hiding this comment.
sorry I should've been more clear, tests related to to_image failed.
There was a problem hiding this comment.
I think I got it better now. So according to https://github.com/rdkit/rdkit/blob/c43f6d363a7aab6d4010bb50076c06608d5671be/rdkit/Chem/Draw/IPythonConsole.py#L247, maxMols is set to 50 by default and only available/triggered when running within IPython.
It sounds a bit strange to be honest, I wonder whether we should disconnect maxMols from the datamol max_mols and maybe connect it to a new argument max_mols_ipython and set it to the same default as rdkit (50).
It might sound complicated but it seems to better match what rdkit is doing and I feel it's best to provide a transparent and flexible interface to the downstream users.
What do you think?
There was a problem hiding this comment.
That sounds good to me. Might be worth adding a logger warning to alert the user that they should be using max_mols_ipython if they want to plot >50 mols?
There was a problem hiding this comment.
Yes good idea. Either a warning or make it clear in the docstring. Thanks!
datamol/viz/_viz.py
Outdated
| @@ -3,6 +3,7 @@ | |||
| from typing import Tuple | |||
| from typing import Optional | |||
| from typing import Any | |||
| from IPython.core.getipython import get_ipython | |||
There was a problem hiding this comment.
We already have dm.viz.utils.is_ipython_session() and also I would prefer to avoid an IPython import at the top level (so it remains optional).
There was a problem hiding this comment.
ah nice, great to know!
|
just got back from a long vacation, I'll get on it ASAP! |
datamol/data/__init__.py
Outdated
There was a problem hiding this comment.
it seems like the gh runner is using a newer version of black and it's forcing the changes here as well as in datamol/viz/_viz.py
There was a problem hiding this comment.
Can you fix the black version in the requirements and the gh (to the newer version, should address the issue)
There was a problem hiding this comment.
updated, lmk if it looks good to you
datamol/data/__init__.py
Outdated
There was a problem hiding this comment.
Can you fix the black version in the requirements and the gh (to the newer version, should address the issue)
Changelogs
closes #221
max_molsis passed toDraw.MolsToGridImageChecklist:
feature,fixortest(or ask a maintainer to do it for you).discussion related to that PR