yscokgor / Greville

Test repository for team
0 stars 0 forks source link

Team Malthus Code Review #2

Open angiechen17 opened 8 months ago

angiechen17 commented 8 months ago

Hello Team Greville,

Overall great job! This was definitely an interesting project and I could tell you put in lots of time and effort into your code. Here's Team Malthus' code review and feedback:

  1. General:

    • Adding more Markdown headers, descriptions, and summaries in your code would help with readability and with emphasizing key findings from the graphs
    • I think changing the file names (especially "Untitled2") to be more representative of what's inside would be helpful
  2. ReadMe.rmd:

    • I was slightly confused when reading the last two paragraphs, and think they could be a little bit clearer with a few small changes. For example, at first I thought the analysis was going to focus on Zimbabwe, not Botswana, so including Botswana in the second to last paragraph would be great!
    • For the "How to Navigate" section, adding commas or using bullet points instead would also help with clarity.
  3. Ag_production:

    • Since this is from the lecture, deleting the unnecessary lecture descriptions would clean your notebook up. It may also be wise to cite any code you copied
    • Since you're focusing on Botswana and Zimbabwe in this project, I would add some graphs focused specifically on those countries
  4. Greville-Population-Tables:

    • Capitalization and grammar issues in the description at the top
    • I would remove some of the empty cells for clarity
    • For the "Women's share of population ages 15+" graph, I would start the x-axis at 1990 since there doesn't seem to be data before then
    • The "Prevalence of HIV, total" graphs use the same indicators but have different y-axis units, so I would standardize that. I was also confused as to why the shapes of the last graphs for Botswana and Zimbabwe were so different from the first graph when they seemed to use the same indicators and data?
    • The population pyramids have negative x-axes units on the left, so I would change those to positive. I would also add titles since it's not clear what the differences are between each graph.
  5. Botswana:

    • All of the graphs look to be duplicates of the Greville-Population-Tables? Should this file exist?
    • A lot of the wb.data functions don't seem to work anymore after the module update, check the documentation to find the correct names
  6. Untitled-2:

    • A lot of the "ls" and "cd" commands seem unnecessary and can be removed
    • The graph produced in cell 44 lacks a title and y-axis labels
    • The title of the graph produced in cell 54 has a spelling error in "Zimbawae". Same with the one in cell 86
  7. Proj1_Code:

    • You imported many of the modules like wbdata, numpy, and plotly.offline multiple times, so I would get rid of those, especially since they take a long time to run.
    • For the date table, some of the values, like for 2022, are NaN, so you should filter those out.

Again, great job Team Greville and good luck on the final presentation!

ligon commented 8 months ago

Great comments! Might be useful for Greville's workflow to have the critical comments each created as a distinct issue.