Skip to content
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

vhmcck/python novice gapminder test branch #619

Conversation

vhmcck
Copy link

@vhmcck vhmcck commented Nov 30, 2022

Summary of changes - attempted second issue

  1. 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.

  2. 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.

  3. Overall restructuring: corrected typos and missing words and rectified the format.

Additional comments and suggestions

  1. 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 ($)’.

  2. 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.

plt.plot(years, gdp_australia, 'g--')

  1. Not sure that we need the ‘{: .language-python}’ after each command/group of commands?

  2. 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.

help(plt.scatter)

@vhmcck vhmcck mentioned this pull request Nov 30, 2022
@alee
Copy link
Member

alee commented Dec 10, 2022

Thanks for the PR! Unfortunately it appears that some of these changes have affected the rendering of the lesson:

image

image

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.

@alee
Copy link
Member

alee commented Apr 20, 2023

Hi @vhmcck unfortunately I didn't end up having the time to go through and fixing the rendering issues with these changes. Please consider resubmitting after we transition to the new Carpentries Workbench later this summer.

Thanks for your efforts to improve this lesson!

@alee alee closed this Apr 20, 2023
@vhmcck
Copy link
Author

vhmcck commented Apr 21, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants