Various showcase changes (mappers, networks, structure, figures,...) #125
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
|
The 'new' showcase notebook is currently named |
hannahbaumann
changed the title
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
Maybe add something like "First we will use rdkit to visualize the TYK2 ligands" or something to prime the reader what we do in the next code block (and so they don't look for an openfe import here)
Reply via ReviewNB
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
We can drop the note I put here "MMH: I think keeping that you can store the network is fine but we don't need more than this + the small code snnipit"
Reply via ReviewNB
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
I agree with your TODO plan, example commands that do all the work we talked about + link to our CLI tutorial.
Reply via ReviewNB
There was a problem hiding this comment.
Changed this!
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
Yes the more I see this now, I think it makes more sense to mention the CLI at the end, and say that you can do all the steps above with these 3 CLI commands. I think it is easier to use the python API to talk about each step, since you can see what is going on with the python code, and its kinda satisfying (I think) for someone following along to hit the end and think "wow some of that python was over my head but I can do all that with a few CLI commands? awesome"
Reply via ReviewNB
There was a problem hiding this comment.
For now I left the part that shows how to dump transformations to JSON here, since I thought this might be useful, but we can also remove it if you think it's too long (since it's also in the tutorial, users could find it there as well).
hannahbaumann
changed the title
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
Sorry it's probably just too early in the morning please ignore if you think otherwise:
"for alchemical substitutions of the TYK2 ligands"
Something about that sentence feels off for some reason? Should it be "of alchemical transformations between the TYK2 ligands"?
Reply via ReviewNB
There was a problem hiding this comment.
You're right, changed this!
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
- Because this is our "showcase", it would be best not to have TODO entries around. Assuming suitable issues are opened for each of these, could we remove them from the user facing text here?
- The diagram we link to here is out of date - I think we should remove it for now and update it later?
Reply via ReviewNB
There was a problem hiding this comment.
Removed these!
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
Should this text re: explanation of mapping viz, go above when we look at atom mappings?
Reply via ReviewNB
There was a problem hiding this comment.
Moved this to the other section!
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
Maybe to be discussed?
Overall this section confuses me a bit because we're doing things in vastly different ways for the two approaches.
Would it make more sense to "just" create the Transformations, and then for the "executing in Python" portion we use "Transformation.create()" to get the DAG out and run that?
Then in the CLI portion we can use the same Transformations, write them to disk and say we can use quickrun to run them?
Reply via ReviewNB
| @@ -2,12 +2,11 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
This is great, thanks @hannahbaumann !
Just the one proposed enhancement but it's very much optional. Please merge when you want!
No description provided.