alee
commented
1 year ago Thanks for the PR! Unfortunately it appears that some of these changes have affected the rendering of the lesson:
Compared to the currently rendered lesson: http://swcarpentry.github.io/python-novice-gapminder/09-plotting/index.html
You should theoretically be able to test your changes locally by running % make docker-serve on your own computer if you have Docker installed. Unfortunately Jekyll is quite finicky about syntax and it may be easier to re-apply the content changes you made to a clean branch.
I should have more time to provide detailed feedback on this next week - again, thank you for taking the time to submit this PR, we just need to work together to deal with syntax issues in the proposed changes.
vhmcck
Summary of changes - attempted second issue
Updated links - besides fixing the issue with the matplotlib link, I have deleted unnecessary (repetitive) links and added new links to this version of the episode (e.g., ‘transpose’ and ‘ggplot’). I also replaced the links to ‘tick_params’ and ’savefig’ as discussed.
Added text to improve clarity, readability, and flow. I personally found the bullet points useful for highlighting important information but not for providing instructions. Learners often need some sort of explanation preceding commands (rather than comments in between lines of code) and prefer small chunks of code. My suggested changes aim to make each step easier to digest.
Overall restructuring: corrected typos and missing words and rectified the format.
Additional comments and suggestions
I believe it would be best to label consistently the two axes in all plots - this is how we promote good habits :) Labels must also be consistent - i.e., ‘GPD per capita’ vs. ‘GPD per capita ($)’.
Many of the examples could build upon their predecessors. For instance, the example with the command below could also include time series, “fancier” styles, axis labels, etc.
Not sure that we need the ‘{: .language-python}’ after each command/group of commands?
Challenges:
Minima and maxima - making separate plots for ‘min()’ and ‘max()’ would be clearer. My proposed changes are included.
Correlations - “the example in the notes” should be provided as part of the instructions;’ describe()’ was not used up until this point in the episode. Also the ‘geopolitics’ explanation for the example of ‘idxmax()’ and ‘idxmin()’ may need more context; I tried to rephrase it but it needs to be polished, perhaps with a link to supporting information.
More correlations - is the command below correct to see the documentation for the ’plt.scatter()’ function? I get a NameError when trying to call it.