-
Notifications
You must be signed in to change notification settings - Fork 1
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
Map plot feat #16
base: add_ggplot2_mapping
Are you sure you want to change the base?
Map plot feat #16
Conversation
Create new supporting functions: - filter_baseline_group - fmt_pos_diff - fmt_census_var_short - plot_demo_lollipop plot_demo_lollipop is also supported by: - annotation_demo_lollipop - demo_text_label - theme_demo_lollipop - scale_color_demo_pos_diff Also inherit citation details
Allow support for mapping w/ ggplot2 as well as tmap Add helper functions: - tm_plot_bias_map - plot_bias_map - fmt_col_to_plot - prep_geo_df
- Update plot_geo_bias_map and tm_plot_geo_bias_map to share docs - Fix issue w/ tmap returning specs not map
- Add str_to_label function - Add GeomLollipop and geom_lollipop and symmetric_limits functions - Rename fmt_census_var_short to fmt_census_var_label - Add aesthetics argument to scale_color_demo_pos_diff - Simplify fmt_pos_diff - Expose new pct_abb argument for create_demo_chart
Also add missing ggplot2 package namespacing
R/utils.R
Outdated
@@ -1,3 +1,17 @@ | |||
#' Citation for Spatial Equity Data Tool API Data | |||
#' | |||
#' @name setd-citation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want this to be sedt-citation (t after d)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Not my first time with this specific typo, regrettably!
R/create_demo_chart.R
Outdated
@@ -9,203 +9,308 @@ | |||
#' @param file_path (character) - Default set to "dem_disparity_chart.png". | |||
#' A file path of where to save the file. This should include a data type | |||
#' suffix. EX: "results/visuals/dem_disparity_chart.png" | |||
#' @inherit setd-citation details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sedt - see note below
save_chart = FALSE, | ||
file_path = "dem_disparity_chart.png", | ||
pct_abb = "Pct.", | ||
ggsave_args = list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why, but when I run save_chart = TRUE
with the default args, I get Error in !ggsave_args : invalid argument type
Consider:
library(sedtR)
withr::with_tempdir({
# Download bicycle data:
download.file(
"https://equity-tool-api.urban.org/sample-data/minneapolis_bikes.csv",
destfile = "bikes.csv")
# Call sedt API with wrapper function
sedt_response <- sedtR::call_sedt_api(
resource_file_path = here::here("bikes.csv"),
resource_lat_column = "lat",
resource_lon_column = "lon",
geo = "city",
acs_data_year = "2021")
})
create_demo_chart(sedt_response$demo_bias_data, save_chart = TRUE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly forgot ggsave()
doesn't support splicing a list of arguments. I added in rlang::exec()
to execute ggave with the spliced arguments.
if(save_map){ | ||
tmap::tmap_save(tm = bias_map, | ||
filename = stringr::str_c(file_path, file_suffix)) | ||
pkg <- arg_match0(pkg, c("tmap", "ggplot2")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add check for if interactive, pkg cannot be ggplot2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is set up this way, then interactive = FALSE
should be the default. Otherwise, switching to using ggplot2 requires switching the pkg
argument and setting interactive
.
#' “Spatial Equity Data Tool API” (Version X.x.x). Washington, DC: Urban Institute. | ||
#' https://ui-research.github.io/sedt_documentation/. Data originally sourced from various sources, | ||
#' analyzed at the Urban Institute and made available under the ODC Attribution License. | ||
|
||
create_map <- function(geo_df, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very supportive of having helper functions in create map that work separately for ggplot2
and tmap
. That being said, I don't think it makes sense to have multiple user-facing functions. I'd rather have everything work through a unified create_map()
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is the preference, then I'd like to make create_map()
a little more flexible so it allows you to set the palette and makes the file path argument optional. I think keeping the helper functions as internal would be a fair compromise – that way I (or other developers) can access them directly without using :::
.
data = geo_df, | ||
fill_col = col_to_plot, | ||
fill_palette = pal, | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to pass on interactive = interactive
here or else it is always interactive = FALSE
tmap::tm_borders( | ||
lwd = border_lwd | ||
) + | ||
tmap::tm_tiles( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: this results in basemap going over the choropleth.
Two potential solutions:
-
tmap::tm_tiles() argument should be deleted. This puts the basemap over the choropleth.
-
Its possible we use a labels-only layer. in this case, maybe for simplicity we mandate the labels be from a specific map:
tmap::tm_tiles("CartoDB.PositronOnlyLabels", group = "Place Names") +
fill_col = "diff_pop", | ||
fill_label = "Disparity Score", | ||
fill_palette = "RdBu", | ||
fill_scale = ggplot2::scale_fill_distiller( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this code as is, I get an error related to this fill_scale:
create_map(sedt_response$geo_bias_data, pkg = "ggplot2")
leads to:
<simpleError in if (!palette %in% unlist(brewer)) { cli::cli_warn("Unknown palette: {.val {palette}}") palette <- "Greens"}: the condition has length > 1>
I believe this is related to the fact that this argument calls the fill_palette
argument which it appears is unsuccessful.
The code works as expected when I do:
fill_scale = ggplot2::scale_fill_distiller(
type = "div",
palette = fill_palette,
labels = scales::label_percent(scale = 1),
direction = 1
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatedly, this has a diverging palette but it is not set at zero. We should consider using scale_fill_gradient2(midpoint=0)
but this will entail some refactoring of the palette
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should be able to get to this next week.
No description provided.