-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs examples #45
Docs examples #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samgdotson, great PR.
Quick think I noticed was that in certain places in the docs you write Osier
and in other places you put osier
. I think it is best to be consistent with the capitalization of the software, but maybe you did this intentionally.
Some of the cells in the example notebooks are rendering strangely. See comments below.
}, | ||
{ | ||
"cell_type": "code", | ||
"execution_count": null, | ||
"metadata": {}, | ||
"outputs": [], | ||
"source": [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | |
{ | |
"cell_type": "code", | |
"execution_count": null, | |
"metadata": {}, | |
"outputs": [], | |
"source": [] | |
} | |
} |
Get rid of empty cell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like some of your headings and sub headings are not following consistent capitalization rules. I have recommendations for further things to add, which I will make issues for.
"with plt.style.context(\"dark_background\"):\n", | ||
" plt.plot(hours, demand, color='cyan')\n", | ||
" plt.ylabel('Demand [MW]')\n", | ||
" plt.grid(alpha=0.2)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add x labels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add x and y axis labels to all figures that do not have them. For the graph of demand and wind vs time(?), you can use a double y axis to label them.
Thanks for the review @yardasol!
I thought I capitalized |
I think I'll convert this PR to a draft PR -- seems like more work needs to be done. @yardasol I am curious about the build system issue with pandoc, though. Can you install it that way? I recall having issues with it... |
Thanks for the review @smpark7! I added most of your suggestions to the notebooks. I didn't change the order of the capacity expansion notebook -- but I did clarify each of the problems covered in the notebook. Hopefully that's fine for now. As for numbering, I know that's what you do in the moltres docs, but it's not a pattern I committed to here and it seems like mostly a matter of preference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes to the technology tutorial section, I think you've satisfied the other two learning objectives. I've left some other comments for requested changes, but I don't believe any of it is big enough to create a new issue over.
"with plt.style.context(\"dark_background\"):\n", | ||
" plt.plot(hours, demand, color='cyan')\n", | ||
" plt.ylabel('Demand [MW]')\n", | ||
" plt.grid(alpha=0.2)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add x and y axis labels to all figures that do not have them. For the graph of demand and wind vs time(?), you can use a double y axis to label them.
@samgdotson Good work on addressing my comments.
Yes that is good enough
That's fair too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvements, found a few more things to work on.
}, | ||
{ | ||
"cell_type": "code", | ||
"execution_count": null, | ||
"metadata": {}, | ||
"outputs": [], | ||
"source": [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address this.
Thanks for the reviews @smpark7 @ZoeRichter @yardasol. I tried to address as much as I could, some things I simply don't agree with. For instance, tone. It makes sense to have a formal tone in the API documentation, but I don't see a reason to enforce formality in a more instructional notebook. The main request I saw was the addition of labels to all axes and all plots, as well as removing empty cells at the end of the notebooks. Both requests have been fulfilled. If you're satisfied, please merge whenever. |
…ender on github, now.
Nudge @ZoeRichter @yardasol @smpark7 |
Go ahead and merge if yall approve too |
@ZoeRichter @yardasol nudge |
@ZoeRichter @yardasol nudge. @smpark7 if you're satisfied you can merge. |
Merging is blocked because of the requested changes. You will have to dismiss their reviews first. |
Comments addressed, re-review delayed.
@smpark7 try now? |
Now it seems merging is blocked for another reason? |
The was an issue with the ruleset preventing merging. The ruleset has been fixed (readthedocs build is done through read the docs, not GitHub, so I changed the source of the status check. Nothing has been removed). |
If there's no other issues that @yardasol wants addressed, I can merge now. |
Given that you addressed their comments and Olek's status indicates that he is currently away, I'm merging this PR. |
This PR adds an initial example notebook for osier and adds more guidance on how to install linear programming solvers on different operating systems.
Edit: This PR actually adds
twothree example notebooks. One simple example introducing theosier.Technology
class, a second example introducing theosier.DispatchModel
, and a third demonstrating theosier.CapacityExpansion
model.To reviewers: Please do not request more examples in this PR. Instead, please make an issue describing the example you would like to see! Documentation of this type will never be complete. Thanks.
Edit:
Closes #48 and Closes #49