-
Notifications
You must be signed in to change notification settings - Fork 5
Update outlier section of performance report for sales val changes #131
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
Update outlier section of performance report for sales val changes #131
Conversation
| "Algorithm: Outlier, High price", | ||
| str_detect(sv_outlier_reason, "Low price") ~ | ||
| "Algorithm: Outlier, Low price", | ||
| TRUE ~ sv_outlier_reason |
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.
Took a stab at removing the redundant combo of str_detect and regex here.
| make_triad_map("North") | ||
| ``` | ||
|
|
||
| ::: |
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.
Exact same map code as res
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.
Tested and confirmed that the report runs and renders these charts correctly!
jeancochrane
left a comment
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.
Looks great! I hope it's OK that I took the liberty to run the ingest, push to the DVC remote, and update the hashes in the lockfile here. That way when we merge this, everyone else can use dvc pull to grab the version of the training data that includes nonliveable spaces in sv_outlier_reason.
The error we've been seeing in rendering the performance report is definitely unrelated to this PR, so let's go ahead and merge this in the meantime.
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
* Update lockfiles to ggplot2 v4 * Upgrade plotly to 4.11.0 to see if it has better support for latest ggplot * Make sure to ungroup outlier ratio comparison before computing range in `_outliers.qmd`
jeancochrane
left a comment
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.
Great work, sorry this was so much trouble to test due to #133!
Same as the res pipeline change
This PR reworks the performance report to work with out new sales val data model additions and add two things to the report:
It also reorganizes the outlier reasons that are displayed
Closes ccao-data/model-res-avm#426.