Include plot in introductory information to illustrate concept#177
Conversation
andreww
left a comment
There was a problem hiding this comment.
All looks good to me. I wonder if it's worth including the output (to the terminal and/or the plot) in the examples in the quickstart but that's orthogonal to this change. Feel free to merge.
|
Strikes me that we don't have "rules" written down anywhere about who should merge PRs, just a requirement to have them reviewed. I'm happy for @sadielbartholomew to hit merge for this but that's something we should probably document in contributing... |
|
Thanks for the review, @andreww. All good points - I am going to add a few commits to address them.
Yes it would be good to have a rule-of-thumb, even if it isn't written down. I personally like the rule of 'the merger is the PR author' (who can merge once they have at least one approval review or as per whatever review rules) since it's simple and keeps responsibility with those already considering a given PR. So that would be my suggestion. |
|
OK have updated in line with Andrew's good feedback - ready for re-review to sanity check the updates, else a fresh review. |
andreww
left a comment
There was a problem hiding this comment.
Everything looks good apart from the plot image in the rendered quickstart document (https://cats--177.org.readthedocs.build/en/177/quickstart.html) is showing (for me) as text and doesn't show the actual plot. I think this is a markdown v's restructured text issue. I'll take a look and see if I can quickly figure out the fix.
Use restructured text not markdown
|
All good @sadielbartholomew - happy for you to hit merge. |
After #171 updated the plot from POC form, it is nice to showcase it in the introductory information, with a brief note about how the optimal window is where the area under the curve is minimised, as a neat visual demonstration of the overall concept of carbon intensity minimisation which is the key concept for CATS. Namely:
Also moves the GIF to the
docs.source/_staticdirectory, updating any links to that path (just two), since all other images and media are there, as with the example plot which I generated and committed. Alternatively we could have a root-level 'media' directory, but with minimal media I don't think this is necessary.