Fix broken network cookbook with newest konnektor updates - #324
hannahbaumann wants to merge 21 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
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.
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,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.
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.
There was a problem hiding this comment.
keeping it as-is is fine by me!
| @@ -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
There was a problem hiding this comment.
Do you mean making it a public method ones we add API support for this in openfe? Or just here for the cookbook?
Added the docstring!
There was a problem hiding this comment.
the former! nothing to do here.
| @@ -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.
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
There was a problem hiding this comment.
True, I changed this to now remove the files at the top.
| @@ -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
There was a problem hiding this comment.
I added this in the description and also moved the import to right above the function to make it clear.
| @@ -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
There was a problem hiding this comment.
I think Josh and my idea here originally was to try and see how an API point could look like, so that's why it's a function, but I'm also happy to in-line it if that was more readable.
There was a problem hiding this comment.
If we're adding this to the docs, then I would go with doing it in-line, it's more "here is something a user should be able to understand".
There was a problem hiding this comment.
Ok, I changed this!
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of comments, nothing blocking. Will approve early.
…eEnergy/ExampleNotebooks into fix-broken-network-new-konnektor
4 participants
No description provided.