Fix broken network cookbook with newest konnektor updates - #324
hannahbaumann wants to merge 17 commits into
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
hannahbaumann
changed the title
hannahbaumann
changed the title
| @@ -0,0 +1,853 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Rephrased this!
| @@ -0,0 +1,853 @@ | |||
| { | |||
There was a problem hiding this comment.
A little bit more prose here would be good, i.e. "we start by loading our LigandNetwork, which can be found in mst_network_tyk2.json..."
Reply via ReviewNB
| @@ -0,0 +1,853 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #7. plot_atommapping_network(planned_network)
If you capture the figure, I think it'll only plot the output once
Reply via ReviewNB
There was a problem hiding this comment.
Good point, I changed this!
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
I mostly reviewed for content - overrall looks good to me, just a couple of small comments on the amount of details that are in the text.
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Make it clear this is specific to the hybrid topology protocol currently (the way we get the protocol and build the transformations) but the process of identifying missing results and creating new transformations should apply to other RBFE protocols?
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
"A transformation counts as successful only when both its complex and solvent legs are completed successfully." We should make it clear that this is specific to the Hybrid Topology Protocol and that the settings helper function below is specific for that one as well or consider making it general?
Reply via ReviewNB
There was a problem hiding this comment.
Thanks, I changed this. I think since this is just an example notebook and we'll be working on the API version soon, it's not necessary to make this general here.
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #4. mapper: AtomMapper,
To consider, should this have the same API as the network generators if this would eventually go into Konnektor and support multiple mappers and a scorer?
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #5. ("lig_ejm_31", "lig_ejm_46"),
Is it intentional to generate the same edges again that we faked failing? We also don't do anything with new edges by name should we check this matches what we intened?
Reply via ReviewNB
There was a problem hiding this comment.
No, I think I had just picked these from the order in the network, but you're right, it's better to intentionally take edges that were not the ones that we said failed. Changed this!
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #39. cofactors = {
For cofactors, we could also use get_components_of_type SmallMoleculeComponent and then remove those in the mapping, this feels a little safer than going on the cofactor label?
Reply via ReviewNB
There was a problem hiding this comment.
Thanks, changed this!
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
[nit] I'd prefer using "broken" instead of "damaged" just for consistent language everywhere.
Reply via ReviewNB
There was a problem hiding this comment.
Changed this!
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Removed this!
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
maybe it makes sense to move the reading of the reference chemical system to later, at the point where you actually need it?
Reply via ReviewNB
There was a problem hiding this comment.
That would also be possible, I'm just not sure if it would help the users since we'd have to parse through the results again. If you have a srong preference, I'm happy to change it, but I think I would slightly prefer opening the files only once.
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
I'm tempted to make load_result_json a public method, but maybe we discuss that for v1.14/15.
_units() needs a docstring. Personally, I would find type-hints helpful for these functions, but I'm not sure if that's true for our targeted audience here.
Reply via ReviewNB
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
a few more inline comments could make this more digestible for users wanting to augment this code.
Reply via ReviewNB
There was a problem hiding this comment.
Added some more inline comments.
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks, changed this!
| @@ -0,0 +1,808 @@ | |||
| { | |||
There was a problem hiding this comment.
I think parallel structure with the beginning would help here, something like:
Identify failures: campaign results -> broken ligand network
Repair the ligand network: broken ligand network -> new repair edges
Build replacement transformations: new repair edges -> runnable transformations.
Reply via ReviewNB
There was a problem hiding this comment.
Thanks, changed this!
atravitz
left a comment
atravitz
left a comment
There was a problem hiding this comment.
overall this looks great (and makes me excited to see how much simpler I can make it using ResultsNetwork).
requesting changes to improve consistency & clarity.
| @@ -0,0 +1,810 @@ | |||
| { | |||
There was a problem hiding this comment.
[nit] Can you put backticks around objects, e.g. Transformations -> Transformations ?
Reply via ReviewNB
| @@ -0,0 +1,810 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #21. if not any(a in path.name and b in path.name for a, b in DROP_EDGES)
[nit] I wonder if this woulld be more readable to readers if we just didn't have to drop results, i.e. if the files were just missing to begin with? Could just move this to an rm call at the same time you open up the tarball?
Reply via ReviewNB
| @@ -0,0 +1,810 @@ | |||
| { | |||
There was a problem hiding this comment.
[nit] because the imports were so long ago, it might be good to remind readers of where MstConcatenator comes from.
Reply via ReviewNB
| @@ -0,0 +1,810 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. def repair_network_by_score(
[nit] Is this method being re-used later? If not, it might be nicerr for the reader to not have these four lines in a separate method but rather just in-line on the same cell.
Reply via ReviewNB
| @@ -0,0 +1,810 @@ | |||
| { | |||
There was a problem hiding this comment.
Maybe add some kind of text blurb about next steps? Maybe a link to the quickrun tutorial?
Reply via ReviewNB
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of comments, nothing blocking. Will approve early.
4 participants
No description provided.