-
Notifications
You must be signed in to change notification settings - Fork 10
CLI charging update #187
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
CLI charging update #187
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
IAlibay
left a comment
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.
couple of things otherwise lgtm
| @@ -0,0 +1,103 @@ | |||
| # Generating Partial Charges with the OpenFE CLI | |||
|
|
|||
| This tutorial will show you how to use the OpenFE CLI to generate and store partial charges for a series of ligands | |||
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.
The order of the intro feels a little bit off. I think I would lead with "you need consistent charges".
Then say "by default the behaviour is X".
Then say "here we present tooling to allow you to do this ahead of time, reducing overheads and further improving reproducibility".
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.
nice I think that does flow better!
|
|
||
| Generating partial charges with the `am1bcc` method can be slow as they require a semi-empirical quantum chemical calculation, | ||
| we can however take advantage of multiprocessing to calculate the charges in parallel for each ligand which offers a | ||
| significant speed-up. The number of processors available for the workflow can be specified using the `-n` flag: |
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'd probably go as far as spell it out to readers and go "For example to spread out the calculation over 4 cores:"
| Partial Charge Generation: am1bcc | ||
| ``` | ||
|
|
||
| The full range of partial charge settings can be found in the snipit bellow, note that some may require installing extra packages. |
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.
snippet?
IAlibay
left a comment
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.
Two extra things, otherwise lgtm thanks!
| As such both the `plan-rbfe-network` and `plan-rhfe-network` commands will calculate partial charges for ligands making it expensive | ||
| to run multiple network mappings while finding the optimal one for the resources available. | ||
|
|
||
| Here we present a CLI tool to allow you to do this ahead of time, reducing overheads and further improving reproducibility. |
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.
"do this" -> calculate partial charges
There's nothing wrong with this sentence, here my suggestion is purely based on the assumption that most readers don't tend to have enough time to link the context of multiple paragraphs together.
| @@ -0,0 +1,105 @@ | |||
| # Generating Partial Charges with the OpenFE CLI | |||
|
|
|||
| It is recommended to use a single set of charges for each ligand to ensure reproducibility between repeats or consistent | |||
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.
Do you want to link to Meghan's preprint here?
|
Going to ignore the lomap errors for now and fix them in another PR. |
This PR adds a CLI tutorial for the new
charge-moleculescommand and updates other tutorials to include information on the new default charging procedure, which has been added to the network planning CLI. The python API tutorial has also been updated to keep in line with the CLI updates.