NewYorkCityCouncil / vacant_storefronts

Analyzing vacant storefronts dataset
0 stars 0 forks source link

Code Review Request #1

Open rhirotacouncil opened 1 year ago

rhirotacouncil commented 1 year ago

Code review request for Storefronts_ct_map.Rmd and Storefronts_cd_map.Rmd.

Some things:

Let me know if anything is confusing!

romartinez-nycc commented 1 year ago

Reorganized the repo to follow the project template structure.

Storefront_cd_map.rmd

No issues in running the code, I was able to run it all the way through 🎉

Code efficiency suggestions

Vroom::vroom() is a bit faster if you'd like to use it. https://github.com/NewYorkCityCouncil/vacant_storefronts/blob/ff67d0075566162eeb91870cb2583cb922fe9f0d/Code/Storefronts_cd_map.Rmd#L31

Code documentation

It's helpful to comment on how you are validating your bin choice for the variable being mapped. (There is a small typo, the column is vacant_2022.) https://github.com/NewYorkCityCouncil/vacant_storefronts/blob/ff67d0075566162eeb91870cb2583cb922fe9f0d/Code/Storefronts_cd_map.Rmd#L79-L90

pal_cd <- colorBin( palette = nycc_pal("warm")(5), bins = int_cd$brks, domain = vacant_cd.shp$vacant_2021, na.color = "#E6E6E6", )


####  Map Improvements 

1. Legend 

Above, I used the existing council red-warm palette from the webpage, but open to discussing whether we should change it to the council blue. I don't have a preference.  For `nycc_pal("warm")(5)`, we should try to limit our bins to a max of 7, drawing from behavioral science, a legend with more than 7 can be too much for the human brain to take in.  For the census tract map, since the tracts are small, we can consider playing with a continuous range of colors (heatmap style) over one that has been divided into bins of discrete colors.

2.  No need to define the following leaflet parameters:

   - min/max zoom (were recently added to the`addCouncilStyle()` function thanks to James!)
   - setView
   - white background 
   - It's easier to leave the height & width parameters undefined at this stage. We can adjust the size within the WordPress iframe stage for the HTML output or within mapshot for the static export ex.`vwidth = 900, vheight = 870.` https://github.com/NewYorkCityCouncil/vacant_storefronts/blob/ff67d0075566162eeb91870cb2583cb922fe9f0d/Code/Storefronts_cd_map.Rmd#L99-L116

You may need to reinstall the councildown package to pull in the latest updates
`remotes::install_github("newyorkcitycouncil/councildown")`

####  Map Suggestions

1. I tend to read in datasets into leaflet, for example, the `vacant_cd.shp` dataset within the addPolygon or other addLealfet functions over including it in the main leaflet() section. This is most helpful when mapping multiple datasets, for example, if you also wanted to add boro polygons, it may lead to fewer code issues. https://github.com/NewYorkCityCouncil/vacant_storefronts/blob/ff67d0075566162eeb91870cb2583cb922fe9f0d/Code/Storefronts_cd_map.Rmd#L99
2. Running the map gave me the chance to play around with Anne's new councildown map functions like addLegend_decreasing() and the `highlight_dists =` parameter within addCouncilStyle(). I will share the jpeg of how that ended up looking.