-
Notifications
You must be signed in to change notification settings - Fork 849
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
Mols matrix to grid image #6080
Mols matrix to grid image #6080
Conversation
items in 2D list are in correct position in 1D list, 1D list items (for highlight parameters, the subitems) are not lists
while undoing black autoformatting of existing code
… length, as mols_matrix
I'd like to create parametrized tests to cover a variety of situations. Is there a standard way to parametrize unittests? The three options that I found are:
I didn't find option 2 or 3 in the RDKit codebase. Absent any feedback here, I guess I'll go with 1. to avoid adding a dependency to the RDKit codebase, especially one that may not be that well maintained. P.S. Python has a built-in subTest() context manager but that seems slightly different, not designed to take a range of parameters. |
@bertiewooster : this is still marked as a draft, so I haven't looked at it. Do you intend to come back to it at some point? |
@greglandrum: no need to review it yet. Yes, I intend to come back to it. I was making good progress then my C got confused about which chip architecture my computer has |
Usage examples from MolsMatrixToGridImage docstring:
|
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.
Thanks for the contribution @bertiewooster . I do think this will be useful functionality.
I went through and made a bunch of mainly cosmetic/style suggestions which will make it easier for me to actually review the substance of what's here.
I'll do the real review after that stuff is resolved.
…t, nested iterable.
…d error type if molsMatrix not actually nested.
Hi, I'm re-requesting substantive review because I believe I addressed all @greglandrum's cosmetic/style suggestions above. Thanks! |
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.
LGTM
Thanks @bertiewooster ! |
Great, thanks @greglandrum! I'm excited to have made a functionality contribution to the RDKit. Regarding documentation for this new feature,
|
That would be great!
Yep, that should happen automatically. |
I've got the Jupyter Notebook for explaining
the RDKit version is 2022.03.5:
Running |
Maybe try |
Thanks for the tip! What ended up working was
I initially tried your suggestion as-is and got
so I gathered that part of the problem was the latest RDKit wasn't in the Anaconda channels. |
To the best of my knowledge no version of the RDKit is in the Anaconda channels. It's always been in conda-forge. |
@greglandrum I completed drafting the blog post and submitted a pull request to the RDKit Blog repo. |
Reference Issue
Adds functionality #5917
What does this implement/fix? Explain your changes.
Adds the functionality to use a two-dimensional (nested) data structure as input to create molecular grid images. For example, the following molecular grid could be created by supplying a nested data structure where
Thus, the user can provide a "ragged" data structure (where the length of each data substructure can be different), and the number of columns in the molecular grid image will automatically be set correctly.
Any other comments?