[Bug]: Tuning fails with non-meaningful error message when `tune_settings$measure` real subset of the nuisance parts

MalteKurz commented 2 years ago

Describe the bug

The tune method allows to specify a nuisance specific measure. If the list provided contains a name that is not a nuisance part, a meaningful error message is produced, i.e., tune_settings[['measure']] = list(ml_m = "regr.mae", ml_wrong_name = "regr.rmse") results in something like:

Error in private$assert_tune_settings(tune_settings) : 
  Invalid name of measure ml_m, ml_r 
 measure must be a named list with elements named ml_g, ml_m

However, if the list of measures is a real subset (e.g. tune_settings[['measure']] = list(ml_m = "regr.mae")) of the nuisance parts it fails with a non-meaningful error message:

Error in default_measures(task_type)[[1L]] : subscript out of bounds

Minimum reproducible code snippet

library(DoubleML)
library(mlr3)
library(mlr3learners)
library(data.table)
set.seed(2)
ml_g = lrn("regr.ranger", num.trees = 10, max.depth = 2)
ml_m = ml_g$clone()
obj_dml_data = make_plr_CCDDHNR2018(alpha = 0.5)
dml_plr_obj = DoubleMLPLR$new(obj_dml_data, ml_g, ml_m)
par_grids = list("ml_g" = paradox::ParamSet$new(list(
    paradox::ParamInt$new("num.trees", lower = 5, upper = 6, default = 5))),
    "ml_m" = paradox::ParamSet$new(list(
    paradox::ParamInt$new("num.trees", lower = 5, upper = 6, default = 5))))
default_tune_settings = list(
    n_folds_tune = 5,
    rsmp_tune = mlr3::rsmp("cv", folds = 5),
    measure = NULL,
    terminator = mlr3tuning::trm("evals", n_evals = 20),
    algorithm = mlr3tuning::tnr("grid_search"),
    resolution = 5)

tune_settings = default_tune_settings
tune_settings[['measure']] = list(ml_m = "regr.mae")

dml_plr_obj$tune(param_set=par_grids, tune_settings = tune_settings)

Expected Result

I would expect that the ML methods are tuned successfully. For all nuisance parts where a measure was actively set, I expect it to be used and for all other nuisance parts I would expect that it falls back to the default measure. I expect this behavior, because otherwise it wouldn't make sense to check for subset here https://github.com/DoubleML/doubleml-for-r/blob/acb9d46ee7eece72efff61e6cdc089c69696572b/R/double_ml.R#L1316-L1323.

Alternative expected behavior: As an alternative we could enforce that either for every nuisance part there is measure set or for none (resulting in default measures being used for every nuisance part). If we go for this alternative solution, we should check for exactly matching list keys instead of checking for a subset. This would then produce a meaningful error message. However, I prefer the above described solution where we fall back to default measures for every nuisance part where no measure was actively set (the implementation of this selective fallback solution would be easy).

Actual Result

Error in default_measures(task_type)[[1L]] : subscript out of bounds

Versions

> sessionInfo()
R version 4.0.4 (2021-02-15)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 21.10

> packageVersion('DoubleML')
[1] ‘0.4.1’
> packageVersion('mlr3')
[1] ‘0.11.0.9000’

MalteKurz commented 2 years ago

@PhilippBach Would be great, if you could quickly comment on this. I can then do the implementation, which is quick and easy. Furthermore, I am anyways working on some minor fixes and updates right now.

MalteKurz commented 2 years ago

@PhilippBach Would be great, if you could quickly comment on this. I can then do the implementation, which is quick and easy. Furthermore, I am anyways working on some minor fixes and updates right now.

Please find a fix proposal in #160, see e09408cefc9080fdeeb72f5293969b1719a1878e

PhilippBach commented 2 years ago

Hi @MalteKurz ,

sorry for my late reply!

I would expect that the ML methods are tuned successfully. For all nuisance parts where a measure was actively set, I expect it to be used and for all other nuisance parts I would expect that it falls back to the default measure. I expect this behavior, because otherwise it wouldn't make sense to check for subset here

I agree! That's what I'd expect too and I'd guess that it's probably what most users would expect too! Thanks for proposing the fix! I think it looks good!

DoubleML / doubleml-for-r