Allow ensemble filters to include multiple base filters of the… #2693

annette987 · 2019-11-29T05:48:14Z

PR for Issue #2688

The base methods of ensemble filters currently have to be unique, which limits the types of ensembles that can be created. This PR removes that restriction by:

Passing the base methods as a named list, where the names are identifiers and the values are the filter methods.
Passing the arguments to the filters in more.args as a named list, the same as in the current implementation, but in this case the names are the same identifiers, not the filter names.
Distinguishing an ensemble method from a base method by checking if the name of the filter exists in the FilterEnsembleRegister.

Pass the base methods and the arguments to them as named lists, where the names are identifiers to identify the method used.

…/mlr into ens_filter_duplicates

R/generateFilterValues.R

tests/testthat/test_featsel_filters.R

pat-s · 2019-12-05T23:02:07Z

I'll come to this after #2698 is done.

pat-s

Thanks!

We should wait until the PRs fixing the bugs are merged before merging a feature PR. Meanwhile, you can address the comments but maybe it makes sense to wait until the other PRs are merged to prevent multiple merging efforts.

Also please make sure to apply the mlr-style.

NEWS.md

R/generateFilterValues.R

tests/testthat/test_featsel_filters.R

pat-s

After resolving the merge conflicts and the two formatting related comments, I'd say we're ready to merge.

If you want, feel free to add yourself as an author to the package DESCRIPTION.

tests/testthat/test_featsel_filters.R

…/mlr into ens_filter_duplicates

annette987 · 2019-12-16T05:42:46Z

Sorry, I am really not sure what is going on here. I resolved the conflicts, did and git pull and a git push and the tests failed. I tried to fix things by doing a reset and starting over, but I may have made things worse. I am not sure what to do now. Can you advise please. Thanks.

DESCRIPTION

pat-s · 2019-12-16T10:35:38Z

R/generateFilterValues.R

+    out = melt(out, value.name = "value", measure.vars = colnames(fval),
+      variable.name = "method")


@annette987 This change caused the test failures. It probably slipped while solving the merge conflicts in master.

Actually that change was deliberate and was there before the merge. The reason for it is that method can now have duplicate filter names, which will cause the melt to fail. So I used colnames(fval) because that is set to names(method) if not null and method otherwise. In fact I could have used index_names, which is set to teh same thing.

It could be that this change is still needed then. However, in one of the latest PRs there was the change to variable.name = "filter" which definitely needs to come into this PR here.

(The column name listing all the methods used is no longer called "method" but "filter" now to avoid name clashes in other places.)

If tests fail you should then know why and be able to solve the issue quickly :)

If tests still pass despite your concern with the duplicate filter names, it might be good to add a test to verify the correct behavior of this edge case.

All OK now. It was just the change to variable.name = "filter" as you suggested. I think the test I already added should be sufficient: https://github.com/annette987/mlr/blob/21c3b8b768baa0b1d7fcad5faf1bb38d157d7fbc/tests/testthat/test_featsel_filters.R#L149
Thanks for your patience with a Github newbie! :-)

Thanks for your patience - we know how hard it is to make PRs to packages to comply with everything and especially in mlr 😄

It seems that you need to update man/ again, just run `devtools::document()´ or press CTRL+Shift+D in case you are using RStudio.

☺️
All looks OK now.I did run devtools::document() but it updated what looked like all of the documentation files and I was reluctant to push these. So I have just resolved the conflicts instead.

I'd assume that you are not running the latest version of roxygen?
When developing pkgs, it is important to always have the most recent versions of pkgs - we usually update every few days :)

Oh OK. I will do that from now on. Thanks.

pat-s · 2019-12-18T15:45:38Z

Thanks @annette987 for all your contributions!

annette987 · 2019-12-19T00:16:16Z

My pleasure. I have learnt a lot along the way. Thanks for all your help.
Do I need to click the 'Update branch' button, or should I leave that to you? How do we finalise this?

…org#2693) * Allow ensemble filters to have duplicates of the same base method. Pass the base methods and the arguments to them as named lists, where the names are identifiers to identify the method used. * Fixed problem with 'melt' - column names of fval were incorrect. * Added unit test and news. * Fixed call to mapply so that it cannot e called with a null argument. * Fixed issues requested by reviewer - indentation and spaces around =. * Changes requested in PR * Changes requested in PR 2693 * Styling changes and authorship * Did a reset (not hard) to try and fix problems * whitespace > tabs * format and fix test failures * Changed melt to use filter, not method * fix news * fix filterWrapper doc

annettespooner and others added 6 commits November 26, 2019 16:14

Allow ensemble filters to have duplicates of the same base method.

d86a29b

Pass the base methods and the arguments to them as named lists, where the names are identifiers to identify the method used.

Fixed problem with 'melt' - column names of fval were incorrect.

12ff591

Added unit test and news.

6601e4c

Merge branch 'master' into ens_filter_duplicates

1c0c081

Fixed call to mapply so that it cannot e called with a null argument.

53b8bb5

Merge branch 'ens_filter_duplicates' of https://github.com/annette987…

5660185

…/mlr into ens_filter_duplicates

larskotthoff requested changes Dec 2, 2019

View reviewed changes

R/generateFilterValues.R Outdated Show resolved Hide resolved

R/generateFilterValues.R Outdated Show resolved Hide resolved

tests/testthat/test_featsel_filters.R Show resolved Hide resolved

Fixed issues requested by reviewer - indentation and spaces around =.

2a10e5a

larskotthoff approved these changes Dec 4, 2019

View reviewed changes

larskotthoff requested a review from pat-s December 4, 2019 23:19

pat-s reviewed Dec 6, 2019

View reviewed changes

annettespooner added 2 commits December 12, 2019 12:01

Changes requested in PR

de30a28

Changes requested in PR 2693

541300f

pat-s requested changes Dec 14, 2019

View reviewed changes

tests/testthat/test_featsel_filters.R Show resolved Hide resolved

tests/testthat/test_featsel_filters.R Outdated Show resolved Hide resolved

annette987 and others added 5 commits December 16, 2019 11:22

Merge branch 'master' into ens_filter_duplicates

f886372

Styling changes and authorship

71d9f3d

Merge branch 'ens_filter_duplicates' of https://github.com/annette987…

334a04a

…/mlr into ens_filter_duplicates

Did a reset (not hard) to try and fix problems

e1d5eb6

Merge branch 'ens_filter_duplicates' of https://github.com/annette987…

0c1441f

…/mlr into ens_filter_duplicates

whitespace > tabs

79778bc

pat-s reviewed Dec 16, 2019

View reviewed changes

DESCRIPTION Outdated Show resolved Hide resolved

format and fix test failures

dd8f35f

pat-s reviewed Dec 16, 2019

View reviewed changes

annettespooner and others added 4 commits December 17, 2019 11:21

Changed melt to use filter, not method

f8f9cfa

resolved conflict

21c3b8b

Merge branch 'master' into ens_filter_duplicates

6139948

fix news

2e6d35e

pat-s approved these changes Dec 18, 2019

View reviewed changes

fix filterWrapper doc

4dce1b3

pat-s changed the title ~~Allow ensemble filters to include multiple base filters of the same type~~ Allow ensemble filters to include multiple base filters of the… Dec 19, 2019

pat-s merged commit cc0cc38 into mlr-org:master Dec 19, 2019

annette987 deleted the ens_filter_duplicates branch December 19, 2019 10:57

pat-s mentioned this pull request Dec 20, 2019

Enhancement: Allow ensemble filters to include multiple base filters of the same type #2688

Closed

annette987 mentioned this pull request Jan 8, 2020

Can't pass additional arguments via more.args to simple filter #2709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ensemble filters to include multiple base filters of the… #2693

Allow ensemble filters to include multiple base filters of the… #2693

annette987 commented Nov 29, 2019

pat-s commented Dec 5, 2019

pat-s left a comment •

edited

Loading

pat-s left a comment

annette987 commented Dec 16, 2019

pat-s Dec 16, 2019

annette987 Dec 16, 2019

pat-s Dec 16, 2019 •

edited

Loading

annette987 Dec 17, 2019

pat-s Dec 17, 2019

annette987 Dec 17, 2019 •

edited

Loading

pat-s Dec 18, 2019

annette987 Dec 19, 2019

pat-s commented Dec 18, 2019

annette987 commented Dec 19, 2019

		out = melt(out, value.name = "value", measure.vars = colnames(fval),
		variable.name = "method")

Allow ensemble filters to include multiple base filters of the… #2693

Allow ensemble filters to include multiple base filters of the… #2693

Conversation

annette987 commented Nov 29, 2019

pat-s commented Dec 5, 2019

pat-s left a comment • edited Loading

Choose a reason for hiding this comment

pat-s left a comment

Choose a reason for hiding this comment

annette987 commented Dec 16, 2019

pat-s Dec 16, 2019

Choose a reason for hiding this comment

annette987 Dec 16, 2019

Choose a reason for hiding this comment

pat-s Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

annette987 Dec 17, 2019

Choose a reason for hiding this comment

pat-s Dec 17, 2019

Choose a reason for hiding this comment

annette987 Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

pat-s Dec 18, 2019

Choose a reason for hiding this comment

annette987 Dec 19, 2019

Choose a reason for hiding this comment

pat-s commented Dec 18, 2019

annette987 commented Dec 19, 2019

pat-s left a comment •

edited

Loading

pat-s Dec 16, 2019 •

edited

Loading

annette987 Dec 17, 2019 •

edited

Loading