Closed mbh329 closed 2 years ago
This looks pretty good. I checked the math in excel and it works out. Some minor comments and then a larger point about refactoring
Small comments
accessopenspace
as it's one indicator name Bigger point: I think the three functions you have for aggregating by each geography can be collapsed down to one. This function should take the geography as a single parameter that it uses for the groupby and the print statement. If this was implemented the main accessor wouldn't need an if/else block
Ok those changes look good. However when I pulled your changes it seems to have added a new file instead of replacing the old ones. Definitely need the old files to be deleted before we can merge in
Sorry about that. thought I deleted them earlier, deleted now.
There are still two .py files that appear to do the same thing. How did that happen? You shouldn't be copying files when you change file names, just use the rename in vscode or mv
from command line
I honestly don't know how that happened - I have been renaming in vscode.
Interesting. It could have to do with how you use git. When you type git status it shows a rename as one new file added and the old one deleted. You have to commit both changes. To stage the deletion you can add the folder or specify the file by name. I use git status before/after each add, commit and push to make sure I am seeing what I expect
Pretty simple stuff going on here.
internal_review
Added 3 files following naming conventions, rounding conventions, and folder structure laid out in the wiki
qol_access_to_open_space.py
I was having issues running this locally on my machine because of an
ImportModuleError
withutils
when running the script in the terminal but was able to generate outputs in ipython. The original xlsx came with 4 columns (puma, pop_w_access, total_pop, pct_w_access). I decided to drop the last column and generate the pct by myself because they had to be generated at the borough and citywide level anyway (double checked the pct against original xlsx and pct's matched). Used the main accessor code that sasha had been using and generated the internal review files using theset_internal_review_file.py
(side note but I think we should rename the function or the script as they are just a letter different - i.e.set_internal_review_file.py
vs funcset_internal_review_files()
. I have some ideas about refactoring this script but looking forward to hearing feedback - I think this should be trimmed down and the pct calculation should be done for each geo level in a function