1235-Dashboard-add radio buttons w moving average breakdown by wk and mo, normalize by population per 10k people and update labels #1242
Conversation
Update neighborhood.py
…ly council average
There was a problem hiding this comment.
Thanks for working on this Piero!
Please update the title and description of this PR to reflect the changes that you've made.
Most of my comments are related to readability, and I know some of the issues already existed beforehand. We are working on improving the readability of our code, and we're asking everyone to improve where they can. Sorry for the extra requests, but I think it will be valuable for future contributions!
piotrsan
changed the title
There was a problem hiding this comment.
Thanks for the updates!
Can you include screenshots of the updated dashboard?
Please run an auto-formatter. There are some simple issues with spacing. Also, quote usage is inconsistent. Please use double-quotes throughout the file.
Please add ".idea/*" to ".gitignore". We don't want local IDE configs to be included in the repo
|
|
||
| total_sum_df['nc_ma'] = total_sum_df.counts.rolling(selected_timeframe, center=True).mean()/NEIGHBORHOOD_COUNCILS_NUMBER |
There was a problem hiding this comment.
When we have any type of literal (in this case, "nc_ma") that's being used multiple times throughout the code, it's good to make a constant for it.
There was a problem hiding this comment.
Can you give me an example for this? I am not sure I quite get what you mean.
There was a problem hiding this comment.
You can make a constant like:
NC_MOVING_AVERAGE_KEY = "nc_ma" # Key for neighborhood council moving average in dataframe.
Similar to how you made a constant for the number of neighborhood councils.
There was a problem hiding this comment.
Hi Nich, I implemented this, but I am not sure it is correct. I don't completely understand why I would create a variable name to a column name, other than having a place where I can write a comment about what the variable does. Am I getting this right?
|
Here is the current screenshot of the first part of the Dashboard: Here how it looks with 7day MA. |
|
|
||
| total_sum_df['nc_ma'] = total_sum_df.counts.rolling(selected_timeframe, center=True).mean()/NEIGHBORHOOD_COUNCILS_NUMBER |
There was a problem hiding this comment.
You can make a constant like:
NC_MOVING_AVERAGE_KEY = "nc_ma" # Key for neighborhood council moving average in dataframe.
Similar to how you made a constant for the number of neighborhood councils.
|
It's totally fine for this PR, but in the future, let's split this into two PRs: one for the name change, and one for the moving average change. |
|
Thanks Nich. I have implemented some normalization, should that be made into a new ull request? |
|
Yes, please make a new PR. But before you make the PR, please open an issue explaining why we need normalization |
Hi Nich, sorry I did not make commits between the comment addressing and the population adjustment, next time I will make sure to create a new PR before I start working on the next issue
piotrsan
changed the title
|
Hi Nich, thanks for the comments. I implemented all changes and restricted this particular dashboard for showcasing how a specific council compares to the average (this has to be done by normalizing by population for the comparison to be meaningful). I removed the raw counts, which can be visualized in other dashboards or in a different graph if needed. |
|
The current visualization is done using population data from NC_pop_2020.csv which was generated from arcGS public data analysis (it is not clear how the analysis is done by the app). Anupriya will provide the final numbers through her careful analysis, which can then be integrated into the dashboard. |
|
Closed without merge due to outdated PR & irrelevant to current working branch |
Fixes #{issue number here}
devbranchAny questions? See the getting started guide