add irace #227

RaphaelS1 · 2020-03-09T19:38:57Z

Adds TunerIrace
Adds irace dependency
Adds irace_helpers.R for helper functions to make TunerIrace a bit neater
Adds tests for TunerIrace

Also changed the template very slightly to include all three constructor methods. If you dislike this let me know and I'll revert back.

R/TunerIrace.R

R/irace_helpers.R

R/TunerIrace.R

R/irace_helpers.R

mb706 · 2020-03-09T21:18:34Z

R/irace_helpers.R

+ irace::readParameters(text = par.tab)
+}
+getIraceRange = function(ps){
+ rng = data.table(lower = ps$lower, upper = ps$upper, lvl = ps$levels)


as.data.table() should be enough

What do you mean by enough?

you can probably do something with

as.data.table(ps)[, ifelse(is.na(lower), <whatever paste collapse 'levels'>, paste0("(", lower, ",", upper, ")"))]

Do you know the best way to collapse lists with some elements characters and some not?

as.data.table(ps)[,ifelse(is.na(lower), paste0("(",paste0(levels, collapse = ","),")"), paste0("(",lower,",",upper,")"))]

Doesn't work as it collapses all levels and ignores the `ifelse, e.g. I got:

"c(TRUE, FALSE),NULL,c(\"lvl1\", \"lvl2\"),NULL" "(1,9)"

add by = id and use levels[[1]]:

as.data.table(ps(x = p_dbl(1, 4), y = p_lgl()))[, ifelse(is.na(lower), paste0("(",paste0(levels[[1]], collapse = ","),")"), paste0("(",lower,",",upper,")")), by = "id"]$V1 #> [1] "(1,4)" "(TRUE,FALSE)"

berndbischl · 2020-03-10T00:08:50Z

some small comments for now:

i dislike that you are tagging every irace-control param with "irace". just to be able to add the meta-param "show.irace.output". can we somehow get around this?
your coding style is in many place not mlr3-complaint (arrow, spaces, camel-case, etc). pls fix now
you need to set
properties = "dependencies"
to tell the tuner that it can handle dependent params

berndbischl · 2020-03-10T00:12:17Z

please unit-test paradox_to_irace separately
mlr2 had a lot of unit tests for irace. copy most of them over if possible please

`targetRunner` kept as-is to be in line with `irace` convention

codecov-io · 2020-03-10T10:27:10Z

Codecov Report

❗ No coverage uploaded for pull request base (main@83d8022). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #227   +/-   ##
=======================================
  Coverage        ?   92.30%           
=======================================
  Files           ?       20           
  Lines           ?      390           
  Branches        ?        0           
=======================================
  Hits            ?      360           
  Misses          ?       30           
  Partials        ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83d8022...45ef598. Read the comment docs.

tests/testthat/test_TunerIrace.R

RaphaelS1 · 2020-03-10T10:28:55Z

i dislike that you are tagging every irace-control param with "irace". just to be able to add the meta-param "show.irace.output". can we somehow get around this?
your coding style is in many place not mlr3-complaint (arrow, spaces, camel-case, etc). pls fix now
you need to set
properties = "dependencies"
to tell the tuner that it can handle dependent params

Done, done, done.

please unit-test paradox_to_irace separately
mlr2 had a lot of unit tests for irace. copy most of them over if possible please

Done - Need to check how dependencies are working though. I'm unsure if some tests working as expected. Also added a few dependencies to suggests that could be removed if needed by changing tested learners

RaphaelS1 · 2020-06-11T10:58:16Z

Shall we try to finish, merge and close this PR? @mb706 @berndbischl . Open questions seem to be about correctly setting instances to match mlr3tuning to irace design

jakob-r · 2020-06-15T11:00:23Z

We should first merge #240 and then I will probably have to adapt this PR to the API changes.

RaphaelS1 · 2020-06-15T15:52:15Z

We should first merge #240 and then I will probably have to adapt this PR to the API changes.

okay great, I've converted to a draft for now

RaphaelS1 · 2020-12-02T13:04:25Z

@jakob-r after the merge from #240 (and probably many others since then), what needs to change in this PR?

jakob-r · 2020-12-08T10:03:13Z

Have you merged the master into your branch, check that tests are complete and running, mark this PR as ready for review and we will merge it if everyhting is fine.

RaphaelS1 · 2020-12-08T22:46:00Z

Okay so pinging everyone who replied above many months ago: @jakob-r @be-marc @mb706

I've merged in Master and got it working, however there seems to be a problem upstream causing warnings ('partial match') you can see it on all the builds. For most of the tests I can handle this with suppressWarnings but I can't do this on the primary autotest. The hacky fix would be to catch irace in the autotest and then wrap the public optimize method in suppressWarningsbut that's up to you. I'm 99% sure the problem isn't our side because we don't use length as an argument in a package (grep), but let me know if wrong.

About the tuner itself, some things I'd appreciate double checking before merging, sorry I don't know the internals of mlr3tuning or irace well so a lot of my implementation is unfortunately guesswork:

Is the use of instance correct and concordant with the use in the irace packages? See conversations about this
Currently it's only set-up to work with TerminatorEvals and TerminatorRunTime as the parameters of these are passed to the maxExperiments and maxTime arguments in the irace scenario (see make_scenario in irace_helpers.R), i) do you agree with this?; ii) what does this mean for other terminators? i.e. will they just not work here or do we have to add them in some other way?
Can someone confirm that they agree that minimizing time will work as expected? See lines 43-52 in test_TunerIrace.R for an example
Follow-up from the conversation I had with @mb706 above regarding assign_result, does this still need updating or will the result automatically returned be the 'correct one'. Sorry I'm not very clear here but I'm also not sure at all about the internal of mlr3tuning so it's hard for me to know how the 'best' configuration is returned at the end of tuning (same with irace not sure how this works internally).

Thanks for the help with this

EDIT: For some reason the warnings didn't show up in the tests this time, so it's possibly stochastic? Which seems weird given the nature of the warnings in the previous tests, either way looks good now.

mllg · 2020-12-09T12:33:09Z

I've merged in Master and got it working, however there seems to be a problem upstream causing warnings ('partial match') you can see it on all the builds. For most of the tests I can handle this with suppressWarnings but I can't do this on the primary autotest. The hacky fix would be to catch irace in the autotest and then wrap the public optimize method in suppressWarningsbut that's up to you. I'm 99% sure the problem isn't our side because we don't use length as an argument in a package (grep), but let me know if wrong.

These checks are enabled by our setup.R, i.e. https://github.com/mlr-org/mlr3tuning/blob/master/tests/testthat/setup.R#L4.

You can temporarily disable them if required.

be-marc · 2021-01-15T11:17:32Z

~~I added a new unit test with TerminatorRunTime that seems to run stable.~~ The PR is ready for a review.
The unit test with TerminatorRunTime is still not stable. It rarely fails and is difficult to reproduce. If irace::irace does not return a result, we throw the error message irace::irace did not return a result. The evaluated configurations are still accessible through the archive. The unit test wraps tuner$optimize(instance) in a try statement to avoid failing tests. We just check if the archive has evaluated configurations. Does anyone have a better idea?

jakob-r

Thanks for the effort. Looks good for the most part. Have we discussed it this would be worth an extra package?

R/ObjectiveTuning.R

R/TunerIrace.R

R/irace_helpers.R

jakob-r · 2021-01-18T09:49:59Z

~~I added a new unit test with TerminatorRunTime that seems to run stable.~~ The PR is ready for a review.
The unit test with TerminatorRunTime is still not stable. It rarely fails and is difficult to reproduce. If irace::irace does not return a result, we throw the error message irace::irace did not return a result. The evaluated configurations are still accessible through the archive. The unit test wraps tuner$optimize(instance) in a try statement to avoid failing tests. We just check if the archive has evaluated configurations. Does anyone have a better idea?

Does it fail bc. irace is taking a tiny bit longer than the terminator is allowing?

be-marc · 2021-01-18T15:14:48Z

Our documentation stated

Uninstantiated resamplings are instantiated during construction so that all configurations are evaluated on the same data splits.

I added

If a new resampling is passed, it is instantiated with new data splits. Already instantiated resamplings are kept unchanged.

be-marc · 2021-01-18T15:17:37Z

Does it fail bc. irace is taking a tiny bit longer than the terminator is allowing?

It does not fail often. We substract 5 seconds now. Maybe it helps.

R/TunerIrace.R

Co-authored-by: Jakob Richter <[email protected]>

be-marc · 2021-02-04T10:38:11Z

Closes #63

jakob-r · 2021-03-19T12:53:58Z

tests/testthat/setup.R

- warnPartialMatchAttr = TRUE,
- warnPartialMatchDollar = TRUE
-)
+# The upstream package causes partial match warnings


irace uses rep with length instead of length.out

Reported upstream MLopez-Ibanez/irace#7.

For what is worth, this issue is now fixed upstream so you should be able to re-enable the warnings. We, the irace team (which is basically 2 people doing both teaching and research), are happy to receive issues and pull requests for fixes like these but also for changes that simplify integration with mlr (or other packages).

jakob-r · 2021-03-19T12:59:27Z

I think this can be merged. Any objections?

Raphael added 2 commits March 9, 2020 19:37

add irace

ef51f10

added reference

a08afbe

mb706 reviewed Mar 9, 2020

View reviewed changes

formatting, merging

a40c7a4

Raphael added 3 commits March 10, 2020 08:11

removed parameter tags, fixed paradox2irace

79b7cdb

camel case -> underscore, arrows -> equals, added spaces to ifs

e8659db

`targetRunner` kept as-is to be in line with `irace` convention

tests, params, fixes

c12a293

RaphaelS1 commented Mar 10, 2020

View reviewed changes

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

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

Raphael added 4 commits March 10, 2020 13:26

Update test_TunerIrace.R

bdf853c

Update test_TunerIrace.R

ff64838

Update test_TunerIrace.R

b2adc83

Update test_TunerIrace.R

09d547e

RaphaelS1 marked this pull request as draft June 15, 2020 15:51

Raphael Sonabend added 5 commits December 8, 2020 10:17

Merge branch 'master' into irace

03210a3

update irace

00e0b7e

styler

fbc4924

lint

3787178

clear warnings

04dfc02

RaphaelS1 marked this pull request as ready for review December 8, 2020 22:37

be-marc added the Status: Review Needed label Jan 15, 2021

be-marc added 5 commits January 15, 2021 12:23

Remove mlr3learners

99a8511

Change source

1929b38

Use try in unit test

908d90f

Remove non-ASCII characters

8079a63

Merge branch 'master' into irace

56f3cd6

jakob-r requested changes Jan 18, 2021

View reviewed changes

Review fixes

33ba133

be-marc added 4 commits January 18, 2021 16:18

Remove 5 seconds and update documentation

54f06b4

Add unit test

32c53db

Unit test

5248a9b

Make mlr3 and paradox depends

dcdac67

Base automatically changed from master to main January 25, 2021 19:30

be-marc mentioned this pull request Jan 27, 2021

retrain #277

Closed

Use replicate

315d574

jakob-r requested changes Feb 3, 2021

View reviewed changes

R/TunerIrace.R Outdated Show resolved Hide resolved

be-marc and others added 2 commits February 3, 2021 11:04

Update R/TunerIrace.R

655b6f8

Co-authored-by: Jakob Richter <[email protected]>

Remove browser

74ea443

Merge

45ef598

jakob-r reviewed Mar 19, 2021

View reviewed changes

jakob-r approved these changes Mar 19, 2021

View reviewed changes

be-marc merged commit 5248176 into main Mar 19, 2021

be-marc deleted the irace branch March 19, 2021 13:48

jakob-r mentioned this pull request May 31, 2021

OptimizerIrace mlr-org/bbotk#137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add irace #227

add irace #227

RaphaelS1 commented Mar 9, 2020

mb706 Mar 9, 2020

RaphaelS1 Mar 9, 2020

mb706 Mar 10, 2020

RaphaelS1 Mar 10, 2020

mb706 Feb 12, 2021

berndbischl commented Mar 10, 2020

berndbischl commented Mar 10, 2020

codecov-io commented Mar 10, 2020 •

edited

Loading

RaphaelS1 commented Mar 10, 2020

RaphaelS1 commented Jun 11, 2020

jakob-r commented Jun 15, 2020

RaphaelS1 commented Jun 15, 2020

RaphaelS1 commented Dec 2, 2020

jakob-r commented Dec 8, 2020

RaphaelS1 commented Dec 8, 2020 •

edited

Loading

mllg commented Dec 9, 2020

be-marc commented Jan 15, 2021 •

edited

Loading

jakob-r left a comment

jakob-r commented Jan 18, 2021

be-marc commented Jan 18, 2021

be-marc commented Jan 18, 2021

be-marc commented Feb 4, 2021

jakob-r Mar 19, 2021

be-marc Mar 19, 2021

be-marc Mar 19, 2021

MLopez-Ibanez Apr 14, 2021

jakob-r commented Mar 19, 2021

add irace #227

add irace #227

Conversation

RaphaelS1 commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berndbischl commented Mar 10, 2020

berndbischl commented Mar 10, 2020

codecov-io commented Mar 10, 2020 • edited Loading

Codecov Report

RaphaelS1 commented Mar 10, 2020

RaphaelS1 commented Jun 11, 2020

jakob-r commented Jun 15, 2020

RaphaelS1 commented Jun 15, 2020

RaphaelS1 commented Dec 2, 2020

jakob-r commented Dec 8, 2020

RaphaelS1 commented Dec 8, 2020 • edited Loading

mllg commented Dec 9, 2020

be-marc commented Jan 15, 2021 • edited Loading

jakob-r left a comment

Choose a reason for hiding this comment

jakob-r commented Jan 18, 2021

be-marc commented Jan 18, 2021

be-marc commented Jan 18, 2021

be-marc commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakob-r commented Mar 19, 2021

codecov-io commented Mar 10, 2020 •

edited

Loading

RaphaelS1 commented Dec 8, 2020 •

edited

Loading

be-marc commented Jan 15, 2021 •

edited

Loading