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.

Sign up for GitHub

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

Jump to bottom

Remove legacy ccao::vars_dict attributes from README.Rmd code #4

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Remove legacy ccao::vars_dict attributes from README.Rmd code #4

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe16fc8
Remove legacy ccao::vars_dict attributes from README.Rmd code
jeancochrane Oct 12, 2023
8e9be9d
Lint README.Rmd
jeancochrane Oct 12, 2023
7572d02
Simplify res_params assignment in README.Rmd
jeancochrane Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions README.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,27 +67,30 @@ Because our office (mostly) cannot observe individual condo unit characteristics
library(dplyr)
library(tidyr)
library(yaml)
params <- read_yaml("params.yaml")
library(httr)

condo_params <- read_yaml("params.yaml")
condo_preds <- condo_params$model$predictor$all

res_params_text <- GET(
"https://raw.githubusercontent.com/ccao-data/model-res-avm/master/params.yaml"
) %>%
content(as = "text")
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: No need to do a GET or load httr, read_yaml() can ingest directly from a URL.

Copy link
Contributor Author

@jeancochrane jeancochrane Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ooh that's nice! Simplified in 7572d02 👍🏻

res_params <- read_yaml(text = res_params_text)
res_preds <- res_params$model$predictor$all

condo_unique_preds <- setdiff(condo_preds, res_preds)

ccao::vars_dict %>%
filter(
var_is_predictor,
!var_name_model %in% c("meta_sale_price")
) %>%
jeancochrane marked this conversation as resolved.
Show resolved Hide resolved
inner_join(
as_tibble(params$model$predictor$all),
as_tibble(condo_preds),
by = c("var_name_model" = "value")
) %>%
filter(
stringr::str_starts(var_name_model, "char_", negate = TRUE) |
var_name_model %in% c("char_yrblt", "char_land_sf") |
var_model_type == "condo",
stringr::str_starts(var_name_model, "ind_", negate = TRUE)
) %>%
jeancochrane marked this conversation as resolved.
Show resolved Hide resolved
distinct(
var_name_model,
`Feature Name` = var_name_pretty,
Category = var_type,
Type = var_data_type,
var_model_type
) %>%
mutate(
Category = recode(
Expand All @@ -106,13 +109,15 @@ ccao::vars_dict %>%
)
) %>%
mutate(`Unique to Condo Model` = ifelse(
var_model_type == "condo" |
var_name_model != "loc_tax_municipality_name" & (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit hacky here, but without this line the code will decide that Municipality Name is unique to the condo model simply because the condo municipality feature is called loc_tax_municipality_name while the equivalent residential model feature is called loc_cook_municipality_name. Does this seem reasonable to you?

Copy link
Member

Choose a reason for hiding this comment

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

This is actually a mistake, as both should be using loc_tax_municipality_name if I recall correctly. We should update the res model feature name to reflect this change. @wrridgeway am I correct here?

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a reason they should be different. I think you're correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I opened an issue for the res model fix here! ccao-data/model-res-avm#20

var_name_model %in% condo_unique_preds |
`Feature Name` %in%
c("Condominium Building Year Built", "Condominium % Ownership"),
c("Condominium Building Year Built", "Condominium % Ownership")
),
"X", ""
)) %>%
arrange(desc(`Unique to Condo Model`), Category) %>%
select(-var_model_type) %>%
select(-var_name_model) %>%
knitr::kable(format = "markdown")
```

Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,12 @@ the 2023 assessment model.
| Percent Population Mobility, Moved From Within Same County in Past Year | acs5 | numeric | |
| Longitude | loc | numeric | |
| Latitude | loc | numeric | |
| Municipality Name | loc | character | |
| FEMA Special Flood Hazard Area | loc | logical | |
| First Street Factor | loc | numeric | |
| First Street Risk Direction | loc | numeric | |
| School Elementary District GEOID | loc | character | |
| School Secondary District GEOID | loc | character | |
| Municipality Name | loc | character | |
dfsnow marked this conversation as resolved.
Show resolved Hide resolved
| CMAP Walkability Score (No Transit) | loc | numeric | |
| CMAP Walkability Total Score | loc | numeric | |
| Airport Noise DNL | loc | numeric | |
Expand Down
Loading