+
Skip to content

Conversation

sebffischer
Copy link
Member

No description provided.

@sebffischer sebffischer changed the title Docs2 Improve PipeOpFilter docu Aug 16, 2022
@sebffischer sebffischer requested review from mllg and pat-s and removed request for mllg August 16, 2022 12:15
Copy link
Member

@pat-s pat-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I merged Michels PR before and now there are some conflicts but I guess you should be able to handle them 🙂

R/FilterMRMR.R Outdated
#' library("mlr3pipelines")
#' task = mlr3::tsk("spam")
#'
#' # Note: The filter.frac is selected randomly and should be tuned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave the "the" out here and in all other occasions, just use filter.frac (formatting the argument as code would also be nice).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

R/FilterNJMIM.R Outdated
#' as.data.table(filter)
#' }
#'
#' if (mlr3misc::require_namespaces(c("mlr3pipelines", "MASS", "praznik"), quietly = TRUE)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is exactly is {MASS} needed for in all the examples? Do we only need it in some?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I somehow though rpart was in MASS but it isn't.

README.md Outdated
Comment on lines 68 to 69
## Warning in `[.data.table`(as.data.table(mlr_filters), , !c("param_set", :
## column(s) not removed because not found: [param_set]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this warning

Comment on lines +140 to +143
In many cases filtering is only one step in the modeling pipeline. To
select features based on filter values, one can use
[`PipeOpFilter`](https://mlr3pipelines.mlr-org.com/reference/mlr_pipeops_filter.html)
from [mlr3pipelines](https://github.com/mlr-org/mlr3pipelines).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One sentence per line please :) (maybe worth checking your editor settings, it seems line breaks are enforced at 80 chrs?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is the md file generated from the Rmd

@sebffischer sebffischer marked this pull request as draft August 25, 2022 08:16
@sebffischer
Copy link
Member Author

for some reason I cannot build the README.html because of a http error

Copy link
Member

@pat-s pat-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Seb! 👍

@pat-s pat-s marked this pull request as ready for review September 2, 2022 12:27
@pat-s pat-s changed the title Improve PipeOpFilter docu - Improve PipeOpFilter docu Sep 2, 2022
@pat-s pat-s merged commit 61811d5 into main Sep 2, 2022
@pat-s pat-s deleted the docs2 branch September 2, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载