Repository navigation
Use charged ligands across all tutorials except charging tutorials - #347
hannahbaumann wants to merge 4 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
| @@ -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
| @@ -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.
| @@ -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.
| @@ -89,7 +89,8 @@ | |||
| "\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.
| @@ -31,12 +31,12 @@ | |||
| "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.
| @@ -40,7 +40,8 @@ | |||
| "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
| @@ -11,7 +11,7 @@ | |||
| "tags": [] | |||
There was a problem hiding this comment.
Fixes #333