-
-
Notifications
You must be signed in to change notification settings - Fork 9
Fix assertions for #139 #140
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
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 94.39% 94.46% +0.07%
==========================================
Files 25 25
Lines 464 470 +6
==========================================
+ Hits 438 444 +6
Misses 26 26
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Thanks!
Is this really a breaking change for users?
Most changes are internal and the only user facing one I see is the task_types
renaming in Filter
. But this is not something users typically use. The assertions would fall into category "bug fix" for me.
README.md
Outdated
| carsurvscore | Correlation-Adjusted coRrelation Survival Score | Surv | Integer, Numeric | [carSurv](https://cran.r-project.org/package=carSurv) , [mlr3proba](https://cran.r-project.org/package=mlr3proba) | NA | | ||
| cmim | Minimal Conditional Mutual Information Maximization | Classif & Regr | Integer, Numeric, Factor, Ordered | [praznik](https://cran.r-project.org/package=praznik) | NA | | ||
| correlation | Correlation | Regr | Integer, Numeric | stats | NA | | ||
| disr | Double Input Symmetrical Relevance | Classif & Regr | Integer, Numeric, Factor, Ordered | [praznik](https://cran.r-project.org/package=praznik) | NA | |
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.
The README update sets a lot of values to NA. Is this caused by the PR or a general issue?
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.
I've updated the scripts to generate the table.
library("mlr3filters") | ||
|
||
task = tsk("pima") | ||
task = tsk("sonar") |
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.
Why change the README task?
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.
Pima contains missing values, most filters do not support this. This PR adds an assertion for this.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
Addresses the issues outlined in #139.
The following assertions were missing or broken:
Assertion on correct task_type
Filter$task_type
was documented to take a scalar string, but in practice could have multiple values. Renamed totask_types
(breaking change) and adjusted documentation. Assertion added toFilter$calculate()
.Assertion on non-missing values
Assertion added to
Filter$calculate()
.