Remove legacy ccao::vars_dict attributes from README.Rmd code #4
Conversation
| var_model_type == "condo" | | ||
| `Feature Name` %in% | ||
| c("Condominium Building Year Built", "Condominium % Ownership"), | ||
| var_name_model != "loc_tax_municipality_name" & ( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I can't think of a reason they should be different. I think you're correct.
There was a problem hiding this comment.
Cool, I opened an issue for the res model fix here! ccao-data/model-res-avm#20
There was a problem hiding this comment.
@jeancochrane Looks good! See my tiny nitpicks then this is good to go.
README.Rmd
Outdated
| res_params_text <- GET( | ||
| "https://raw.githubusercontent.com/ccao-data/model-res-avm/master/params.yaml" | ||
| ) %>% | ||
| content(as = "text") |
There was a problem hiding this comment.
nitpick: No need to do a GET or load httr, read_yaml() can ingest directly from a URL.
There was a problem hiding this comment.
Ooh that's nice! Simplified in 7572d02 👍🏻
| var_model_type == "condo" | | ||
| `Feature Name` %in% | ||
| c("Condominium Building Year Built", "Condominium % Ownership"), | ||
| var_name_model != "loc_tax_municipality_name" & ( |
There was a problem hiding this comment.
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?
jeancochrane
force-pushed
the
jeancochrane/use-dbt-dag-for-readme-feature-table
branch
from
6eec8f6 to
7572d02
Compare
jeancochrane
deleted the
jeancochrane/use-dbt-dag-for-readme-feature-table
branch
This PR updates
README.Rmdalong similar lines as the changes in ccao-data/model-res-avm#18 to remove references to legacyccao::vars_dictattributes that were removed in ccao-data/ccao#6. Instead of getting condo-specific attributes from the vars dict, we now pull the latest residential parameters from themodel-res-avmrepo and diff them against the condo params to generate the column marking condo-specific parameters.