+
Skip to content

Conversation

mllg
Copy link
Member

@mllg mllg commented Oct 1, 2022

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 to task_types (breaking change) and adjusted documentation. Assertion added to Filter$calculate().

Assertion on non-missing values

Assertion added to Filter$calculate().

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2022

Codecov Report

Merging #140 (2e54355) into main (143fe1a) will increase coverage by 0.07%.
The diff coverage is 96.96%.

❗ Current head 2e54355 differs from pull request most recent head b203381. Consider uploading reports for the commit b203381 to get more accurate results

@@            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              
Impacted Files Coverage Δ
R/Filter.R 76.66% <90.90%> (+2.59%) ⬆️
R/FilterAUC.R 100.00% <100.00%> (ø)
R/FilterAnova.R 100.00% <100.00%> (ø)
R/FilterCMIM.R 100.00% <100.00%> (ø)
R/FilterCarScore.R 100.00% <100.00%> (ø)
R/FilterCarSurvScore.R 54.54% <100.00%> (ø)
R/FilterCorrelation.R 100.00% <100.00%> (ø)
R/FilterDISR.R 100.00% <100.00%> (ø)
R/FilterFindCorrelation.R 100.00% <100.00%> (ø)
R/FilterImportance.R 100.00% <100.00%> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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!

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 |
Copy link
Member

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?

Copy link
Member Author

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")
Copy link
Member

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?

Copy link
Member Author

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.

pat-s and others added 5 commits October 26, 2022 14:46
@mllg mllg merged commit f30f23f into main Nov 13, 2022
@mllg mllg deleted the fix_assertions branch November 13, 2022 20:17
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.

3 participants

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