Repository navigation
Use charged ligands across all tutorials except charging tutorials - #347
hannahbaumann wants to merge 7 commits into
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
I would remove especially when the molecule is in a conformation far from a local minimum of the AM1 energy landscape. from point 2 and reference the paper as well on that point.
Reply via ReviewNB
There was a problem hiding this comment.
I would add "depending on the partial charge method" or some kind of indication that this is limited to the AM1BCC-based methods that we support.
There was a problem hiding this comment.
Changed this!
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
Is there an easy we can show that there are no charges assigned maybe introduce the to_openff().partial_charges method? This highlights we might want a way on the SmallMoleculeComponent to check this as well.
Reply via ReviewNB
There was a problem hiding this comment.
Thanks, we can add that after that PR is merged!
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
We should highlight that all zero charges are treated as no charges and are overwritten always.
Reply via ReviewNB
There was a problem hiding this comment.
Added this to the overwrite bullet point.
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
Maybe remove including am1bccelf10 (OpenEye toolkit only) and espaloma to avoid it needing changing if we drop espaloma in future?
Reply via ReviewNB
There was a problem hiding this comment.
+1 espaloma is going untested now, so we're only providing very barebones support.
There was a problem hiding this comment.
Removed these.
| @@ -52,7 +52,7 @@ | |||
| " \n", | |||
There was a problem hiding this comment.
I like this style of section with a large heading to highlight the partial charge information, but I am not sure if it should be consistent in all the notebooks or not maybe it doesn't work in some of them but it would be good to make it stand out like this if we could.
Reply via ReviewNB
There was a problem hiding this comment.
I now put the partial charge info into a box in all of the tutorials and notebooks.
| @@ -25,9 +25,7 @@ | |||
| { | |||
There was a problem hiding this comment.
"which can, depending on the selected partial charge method, lead to..."
Also can you open an issue to update this once we get the partial charge validation in? At that point, we can reword all of this to "non-deterministic partial charge methods are not supported at runtime".
Reply via ReviewNB
There was a problem hiding this comment.
Reworded this and raised an issue.
| "metadata": {}, | ||
| "source": [ | ||
| "## 1. Loading the ligand\n", | ||
| "\n", |
There was a problem hiding this comment.
[nit] Could you put this in an info box? I.e. same as what we do in the ccpbiosim tutorial.
If possible / reasonable, please also consider this for all the other notebooks (they all have a very similar "here are some links" phrasing).
Reply via ReviewNB
There was a problem hiding this comment.
Moved this into an info box in all tutorials and notebooks.
| "cell_type": "markdown", | ||
| "id": "7f1a272a-ffe7-4397-a8a6-c590d3a210bc", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
This cookbook doesn't really lead to the creation of a Protocol or anything, i.e. it seems very focused on just networks - in my opinion, it would be ok to just not mention this at all and just use the file as-is.
Reply via ReviewNB
There was a problem hiding this comment.
I still think that since people will likely be using this cookbook to create a network they will want to eventually run, it's still safest to also use the charged ligands here.
| "cell_type": "markdown", | ||
| "id": "39e64f74-337e-4a88-b253-ab051820d5e3", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
| @@ -47,12 +47,13 @@ | |||
| "id": "c07d0639-ebd0-4a9b-ae15-f902edff2850", | |||
There was a problem hiding this comment.
| @@ -176,12 +176,14 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Changed this!
| @@ -35,9 +35,7 @@ | |||
| "source": [ | |||
There was a problem hiding this comment.
[nit] There's maybe a linking mini-sentence missing here. Maybe it's just the start of the sentence that coulduse a small change, something like "Here we load ligands from somebenzenes_am1bcc.sdf that already have partial charges...".
Also, in this case, since we're llilnking to loading small molecules, it's probably ok not to specifically link to the partial charge addition sections (i.e. it avoids having that specific feature "special").
Reply via ReviewNB
There was a problem hiding this comment.
I reorganized this into a info box.
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
I would remove the "generate exactly the charges a Protocol would have generated". That might confuse the reader since above you mention how Protocols adding partial charges isn't a good thing.
Reply via ReviewNB
There was a problem hiding this comment.
Removed this!
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Rephrased this!
|
@IAlibay regarding whether to use the pre-charged ligands in some of the cookbooks, I agree that it's technically not necessary, I just thought that since people may be using these cookbooks to then run calculations with e.g. the created ligand network, it would still be safer to add the partial charge info everywhere and use pre-charged ligands for consistency. But if you think that it would be easier to remove it where it's not strictly necessary, I'm also happy to change it again! |
Just to clarify, I am pro using the same file with pre-charged ligands but I don't think you need to add the extra "we are using charged ligands" everywhere. For the networks etc, having that repeated information essentially makes us concentrate solely on partial charges everywhere. If we want to signpost, I would suggest in those "non-simulation" / charges aren't being used cookbooks that we only signpost to "loading ligands" - that then sufficiently signposts to the partial charge things. Edit: this hinges on the loading cookbook being adequately detailed to be the focus point - I think your updates make it that way. |
|
@IAlibay I now removed the partial charge info box from the ligand network cookbooks, only kept it for the ones that are creating an alchemical network. |
Fixes #333