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

MolsMatrixToGridImage post #16

Merged

Conversation

bertiewooster
Copy link
Contributor

This is a blog post explaining MolsMatrixToGridImage, a new feature in the 2023.09.1 RDKit release.

Copy link
Owner

@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.

Thanks for doing the guest post!
I'm happy to merge this as is, but I would suggest moving the use cases (examples) up before the explanation of each of the parameters. That reference section is useful, but it's not really something super interesting to read through and I think it might be nice to have your (well thought through) examples, which nicely demonstrate why someone might want to use this functionality, show up earlier so that they are more likely to catch peoples' attention.

@bertiewooster
Copy link
Contributor Author

Glad to contribute, Greg! Thanks for your perspective on what's most interesting. Sure thing, I'll reorder those sections.

Move exhaustive example into parameters
Add try...except blocks around cells that are designed to give errors
@bertiewooster
Copy link
Contributor Author

bertiewooster commented Oct 23, 2023

I reordered those sections, and moved the exhaustive example into the parameters section. I also put try...except blocks around the cells designed to give errors, which both allows the notebook to run in its entirety and makes the error messages take up much less vertical space.

It's ready for merging.

Copy link
Owner

@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 77c46aa into greglandrum:master Oct 24, 2023
@greglandrum
Copy link
Owner

Thanks @bertiewooster ! I will try to get this posted later today or tomorrow.

@bertiewooster bertiewooster deleted the MolsMatrixToGridImagePost branch October 24, 2023 23:55
@bertiewooster
Copy link
Contributor Author

Thanks for posting @greglandrum! Nice choice of subtitle and graphic on the blog homepage.

I noticed a few formatting issues, some due to how Markdown was converted to HTML, so I'll submit a follow-up PR. The one that could cause the most confusion is that - were not rendered as bullets below:

A borderline situation is if each row will have the same number of molecules n (n = 2 below): you can create the same grid using either - MolsToGridImage(molsPerRow=n), for example MolsToGridImage([mol1a, mol1b, mol2a, mol2b, mol3a, mol3b], molsPerRow=2). - MolsMatrixToGridImage with n molecules in each row,...

It should appear as:

A borderline situation is if each row will have the same number of molecules n (n = 2 below): you can create the same grid using either

  • MolsToGridImage(molsPerRow=n), for example MolsToGridImage([mol1a, mol1b, mol2a, mol2b, mol3a, mol3b], molsPerRow=2).
  • MolsMatrixToGridImage with n molecules in each row,...

If you've resolved that issue in the past and remember how, please let me know how.

@greglandrum
Copy link
Owner

If you've resolved that issue in the past and remember how, please let me know how.

I will try to clear it up today and update the post

@bertiewooster
Copy link
Contributor Author

Thanks. Could be as simple as putting a blank line before the first '-'.

@greglandrum
Copy link
Owner

Thanks. Could be as simple as putting a blank line before the first '-'.

Yeah, I think that's likely it

@bertiewooster
Copy link
Contributor Author

I made a new PR for the changes.

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.

2 participants