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

Converting pie charts into bar charts in visualization #82

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

swastis10
Copy link

Testing done: By running Jupyter notebooks and comparing them with previous pie charts

Testing done: By running Jupyter notebooks and comparing them with previous pie charts
@swastis10
Copy link
Author

Testing:

image

@swastis10 swastis10 marked this pull request as draft April 20, 2023 00:46
@swastis10 swastis10 marked this pull request as ready for review April 20, 2023 00:46
@swastis10
Copy link
Author

@shankari Please review

@shankari
Copy link
Contributor

shankari commented Apr 20, 2023

@swastis10

  1. the labels overlap with the "last updated" and look ugly
  2. the new bar graphs don't show percentages, or give a sense of the proportions in any other way

@shankari
Copy link
Contributor

By running Jupyter notebooks and comparing them with previous pie charts

Can you explain what you mean by this? How did you compare the charts?

Comment on lines +128 to +129
" data = pd.DataFrame({\"Mode_confirm\": labels_mc,\n",
" \"Number of Trips\": values_mc})\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why you have this copy-pasted before every line.
Can you please explain:

  • why we need to create a separate data frame for the visualization of bar charts when we did not for pie charts?
  • why this needs to be copy-pasted instead being refactored into common code with the same interface?

Comment on lines -252 to -261
"\n",
" #data\n",
" miles_dict = dict(zip(miles['Mode_confirm'], miles['Total (miles)']))\n",
"\n",
" labels_m = []\n",
" values_m = []\n",
"\n",
" for x, y in miles_dict.items():\n",
" labels_m.append(x)\n",
" values_m.append(y)\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this no longer required?

@shankari
Copy link
Contributor

the new bar graphs don't show percentages, or give a sense of the proportions in any other way

The general rule of thumb for replacing pie charts is stacked bar graphs, not bar graphs directly
what is important for the pie charts and their replacements is the proportions, not the actual numbers
using bar graphs directly focuses more on the numbers, not on the proportions

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.

2 participants