Conversation
|
Could you enable Review NB on this repository? That would make reviewing the Jupyter Notebooks a lot easier. You can install it at GitHub Marketplace. The "Free" option for open source works, and you only have to give it access to this specific repository. It will also be useful once I will take a look at the example. |
EwoutH
left a comment
EwoutH
left a comment
There was a problem hiding this comment.
First review round, it looks good in general! I tried to be quite thorough in the review, let me know what you think of it.
Should be done now. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Awesome, I will try to review the notebook tomorrow! |
|
Thanks for the review so far. See my responses. All other issues have been resolved.\ One minor thing: all modifications to optimization.py are part of #193. So once you agree with the responses here, I'll move those changes back into that branch and rebase this one. This branch then should only contain the updated notebook, once #193 has been merged. |
| @@ -98,9 +98,9 @@ | |||
| "name": "stderr", | |||
There was a problem hiding this comment.
where we are going to search over the levers for solutions that have robust performance over a set of scenarios.
Explain/define robust performance briefly and link to further resources.
specify our robustness metrics.
Which robustness metrics are in the workbench? List, link to overview and/or link to each metric.
Reply via ReviewNB
There was a problem hiding this comment.
There are no robustness metrics in the workbench. Robustness if further specified in mcphail to which I already link.
| @@ -98,9 +98,9 @@ | |||
| "name": "stderr", | |||
There was a problem hiding this comment.
robsut --> robust
What we can see in this parallel axis plot is that there is a clear tradeoff between robustly high reliability and robsut low maximum polution
Add how this can be seen (probably diagonal lines, or high density of lines on opposites of the Y-axis between variables)
Aside from only plotting, can we also express these metrics numerically?
Finally, a summary of what is and can be done in this whole notebook needs to be added, with some links and resources where to go further.
Reply via ReviewNB
There was a problem hiding this comment.
- there is a dataframe so yes we have numbers
- no idea what you have in mind and whether it belongs in the documentation.
There was a problem hiding this comment.
Let’s discuss this in person
Yes that's perfect, I probably should have reviewed those changes in #193, I didn't see they were largely overlapping, sorry for that. If you move the changes in #193 I will do a final review on that. I've reviewed the notebook, see https://app.reviewnb.com/quaquel/EMAworkbench/pull/194/files/ |
|
From my perspective all comments on optimization.py are resolved. There are a few points for further discussion outstanding in the notebook. |
|
Perfect, if you port them to #193 I will give it a last look and merge it! |
|
Now that #193 has been merged, we can finalize this as well. There are a view outstanding issues in the notebook. Do you want to go through those in person, or do you prefer to reply in reviewNB? |
|
Most of it looks good, there are two things let I want to discuss in person. would you be able to rebase and clean up before out meeting? Then we can probably merge during it. |
3 participants
Building on #193, this substantially expand the directed search tutorial with improved discusion on convergence, seed analysis and robust optimization. In addition various links to relevant literature has been added.
This pull request has already been rebased on #193, so first we need to merge that one before we review this one.