-
Notifications
You must be signed in to change notification settings - Fork 4
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
Maintain feature parity with model-res-avm
#19
Maintain feature parity with model-res-avm
#19
Conversation
test_data_card <- read_parquet(paths$output$test_card$local) | ||
# recent 10% of sales and already includes predictions. | ||
test_data_card <- read_parquet(paths$output$test_card$local) %>% | ||
filter(!is.na(loc_census_puma_geoid)) |
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.
This should not be here, but I cannot get this stage to run without it.
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 think we actually need to include these additional workflow scripts in the condo model, since the res model workflow can work on both sets of output.
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.
@wrridgeway I'm sure there are lots of little kinks to work out, but overall this looks good. Only changes are to boot the files/chunks that we don't need to duplicate across the repos. Then this should be good to go.
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.
Likewise for this workflow, we can keep it res model only.
dict <- read.csv( | ||
here::here("misc", "file_dict.csv"), | ||
col_types = readr::cols() | ||
colClasses = c("character", "character", "numeric", rep("character", 9)), | ||
na.strings = "" |
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.
Why the switch here? readr
should be in the dependencies IIRC
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.
This is a syntax copied from the res repo
See #18.
This PR is rather extensive. It primarily refactors the pipeline and surrounding infrastructure in order to bring it in line with the res model pipeline.
It's not up-to-date with the last week or so of work in the res pipeline, but this PR is getting pretty unwieldy.