Update repo read me for 2024 #40
Conversation
|
Need to update public s3 data before this gets merged. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@wrridgeway the content additions here are good, but can you fix the line ending issues and re-request review please.
.gitignore
Outdated
There was a problem hiding this comment.
issue: Let's fix the line endings in this file.
There was a problem hiding this comment.
issue: Likewise, for this file: fix the line endings an re-request review.
| | Condominium Building Strata 1 | Meta | character | X | | ||
| | Condominium Building Strata 2 | Meta | character | X | |
There was a problem hiding this comment.
issue: Strata are definitely still features. Why are these booted?
There was a problem hiding this comment.
So, we removed strata from the vars dict here. You okay with me adding it back?
There was a problem hiding this comment.
We should also wrap the current ccao PR before we merge this PR.
There was a problem hiding this comment.
@wrridgeway. Hold up on the current ccao PR. Go check what @jeancochrane did for the res README. We're moving away from needing the vars_dict data to update the model READMEs. The only thing that should be added to vars_dict is stuff that needs to be recoded in the various pipelines. Sorry I didn't remember this sooner.
There was a problem hiding this comment.
I think I'm a little confused. In the res readme there's also an inner_join() against ccao::vars_dict for feature names which seems like it would mean all feature names have to be in that data?
There was a problem hiding this comment.
That might actually be a mistake. I think that join should be reversed, with param_tbl on the left side and vars_dict left joined. Calling @jeancochrane to confirm.
There was a problem hiding this comment.
@dfsnow Yup, I think that is indeed a bug. Looks like we had a conversation about the relevant code block where you suggested a different join method and I copy/pasted your suggestion such that I removed the correct order of the join (which still should have been a left join instead of an inner join). My bad for not thinking it through at the time!
There was a problem hiding this comment.
Alright, in that case I'll work on wrapping up this PR then we can actually revert the CCAO package back a few merges I believe.
|
Line endings fixed, not sure how I missed that. Looking into why strata is missing from the readme now. |
|
Strata is BACK, baby. But I'm not super sure how we want to handle the input data on S3 now. |
There was a problem hiding this comment.
@wrridgeway Some minor language edits here, but then good to go.
No description provided.