Fix broken network cookbook with newest konnektor updates - #324
hannahbaumann wants to merge 15 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!
4 participants
No description provided.