R-Py parity for ml-regression page #63
Conversation
…can by copied and run independently
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
r/2021-07-08-ml-regression.Rmd
Outdated
| y_range <- lm_model %>% predict(x_range) | ||
|
|
||
| colnames(y_range) <- c('tip') | ||
| xy <- data.frame(x_range, y_range) |
There was a problem hiding this comment.
One suggestion here is to rename the dataframe variable to df for this example and other ones (unless there's multiple dataframes).
r/2021-07-08-ml-regression.Rmd
Outdated
|
|
||
| Notice how we can combine scatter points with lines using Plotly. You can learn more about [multiple chart types](https://plotly.com/r/graphing-multiple-chart-types/). | ||
|
|
||
| [knn]: https://scikit-learn.org/stable/modules/generated/sklearn.neighbors.KNeighborsRegressor.html |
There was a problem hiding this comment.
We'll want to replace this with the R example instead.
r/2021-07-08-ml-regression.Rmd
Outdated
| pred <- pred$.pred | ||
| pred <- matrix(pred, dim_val[1], dim_val[2]) | ||
|
|
||
| dim(pred) |
There was a problem hiding this comment.
Do we need this? If not, I would ✂️
There was a problem hiding this comment.
Hey @kvdesai , these examples look great! They're fast and fluid, and I like the syntax used. I was able to run all of them by copying and pasting the code into my local environment and running each example script. The knitted html also looks good.
I've made a few suggestions/comments that would bring it across the finish line. Along with that, I would suggest maybe just taking one more pass over the knitr generated HTML and addressing any of the non-blocking warnings that appear (eg. the geom_smooth formula ones). This will prevent any panic from inexperienced users who may not know what's causing the warning or what they mean for the model.
Other than that, it looks fantastic. I can approve it once those minor changes have been addressed.
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
|
@HammadTheOne - thanks for the review and insightful comments. I have addressed them. I also added one more snippet of the plot. Please verify and let me know if any other change is needed. |
There was a problem hiding this comment.
Looks good! Thanks for making the requested changes.
I notice that one of the CI tests is failing now though (related to 2016-06-17-3d-tri-surf.Rmd, and possibly an update to the anglr package) and specifically this line:
plotly.r-docs/r/2016-06-17-3d-tri-surf.Rmd
Line 103 in eada8db
Once that's fixed and tests are passing we can go ahead and merge it.
💃
No description provided.