Docs #132
Docs #132
Conversation
|
why do we have pre-commit in the ci? |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Why does pre-commit push into my commits? |
|
IDK if you have to touch each example. I think it would be better to demonstrate filtering in the book, and include the URL on the man pages of the filters. You could also plot the graph and show how to extract the filter values and the model. |
|
So basically extend https://mlr3book.mlr-org.com/optimization.html#fs-filter and link here; having a smaller example in the README is also good. |
+1, also because just adding pipelines into the dep stack for this is not worth it.
Why not? It just does what you should have done locally, rendering the man files. |
|
Only if you use roxygen2 syntax. We have similar cases in other extension pkgs where we don't link via roxygen2 if this would be only reason to put a pkg into the dependency stack. I'd really avoid having mlr3pipelines as a dep in here. Instead we can add a sentence into the help files which say "for examples how to use mlr3filters with mlr3pipelines, have a look at this book section". |
|
Why is putting mlr3pipelines in Suggests a problem? This is only one more package in the CI (without any additional dependencies or system libraries), and no extra packages to install for the user. |
There was a problem hiding this comment.
Rethinking this, yes, given that it is an "internal" package the costs are low.
I tried to stick to the general approach of not including a package only for linking reasons in the documentation to keep the dependency chain small.
But we also have some actual use case here, so yes, let's include it.
Regarding examples: rethinking this as well and adding some pipelines snippets might be good. We should document this in the book in addition in more detail.
R/FilterCMIM.R
Outdated
| #' as.data.table(filter) | ||
| #' } | ||
| #' | ||
| #' if (requireNamespace("mlr3pipelines") && requireNamespace("praznik")) { |
There was a problem hiding this comment.
I'd favor using mlr3misc::require_namespaces(c("mlr3pipelines", "praznik")) in cases with n > 1.
There was a problem hiding this comment.
but it needs argument quietly = TRUE then 😞
R/FilterCarScore.R
Outdated
| #' library("mlr3pipelines") | ||
| #' task = mlr3::tsk("mtcars") | ||
| #' | ||
| #' graph = po("filter", filter = flt("carscore"), filter.cutoff = 0.2) %>>% |
There was a problem hiding this comment.
I'd avoid setting filter.cutoff as users might takes this as a "good default" then as its listed in an example.
I see you're setting a different value in each example. Should we calculate a random dynamic value and add a comment that this value should be tuned in practice?
There was a problem hiding this comment.
Adding a comment that this value should be tuned: 👍
I don't think a random value is helping, this is just confusing.
There was a problem hiding this comment.
I don't think a random value is helping, this is just confusing.
Agree, this might not be obvious. Maybe the combination of a hardcoded value across all examples and a comment about the tuning need might be the best middle way.
Addresses #103