Skip to content
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

CondEqual and functions #278

Open
sumny opened this issue Apr 24, 2020 · 9 comments
Open

CondEqual and functions #278

sumny opened this issue Apr 24, 2020 · 9 comments

Comments

@sumny
Copy link
Sponsor Member

sumny commented Apr 24, 2020

Suppose I have a ParamUty accepting a function. I want to have a condition for another parameter on this one, e.g.:

ps = ParamSet$new(params = list(
  fun = ParamUty$new("fun", default = identity, custom_check = checkmate::check_function),
  test = ParamLgl$new("test"))
)

ps$add_dep("test", on = "fun", cond = CondEqual$new(identity))

Currently this fails:

Error in cond$rhs[!feasible_on_values] : 
  object of type 'closure' is not subsettable

Any easy way how I can achieve this? (I guess I could set fun to a character and then later substitute it to evaluate the function with the name of the value of fun but I'd rather prefer to directly work with functions.

@pfistfl
Copy link
Sponsor Member

pfistfl commented Apr 26, 2020

Write a custom condition?
I don't know if this is the intended way, but writing a condition should hopefully only be a few lines.
If we then use a condition several times, we could think about porting it to paradox.

@berndbischl
Copy link
Sponsor Member

@sumny can you please clarify your usecase better? with a concrete example

@sumny
Copy link
Sponsor Member Author

sumny commented Apr 27, 2020

This is for PipeOpTargetTrafoSimple. Here, the ParamSet holds trafo (ParamUty, a function), inverter (ParamUty, a function), new_target_name (ParamUty, a character(1)) and new_task_type (ParamUty, either "classif" or "regr"), see here.

I now want to have the following:
A user should only be able to set a value for new_target_name or new_task_type if trafo is not the identity function. Because if it is the identity, there is an early exit within PipeOpTargetTrafoSimple and both settings will have no effect so from an extra save perspective I should forbid this, i.e., implement a dependency of new_target_name and new_task_type on trafo.

Using CondEqual seems to be problematic because:

  • I need to invert this logically (i.e. I want something like CondInequal).

  • CondEqual uses test = function(x) !is.na(x) & x == self$rhs to test whether the condition is met. However, this will not work if x is a function.

So I could write my own condition, i,e:

CondFunNotEqual = R6Class("CondEqual", inherit = Condition,
  public = list(
    initialize = function(rhs) super$initialize("funnotequal", rhs),
    test = function(x) !identical(x, self$rhs),
    # Printing the name of the function is not possible after assigning so we just print this
    as_string = function(lhs_chr = "x") sprintf("%s = %s", lhs_chr, "_FUNCTION_")
  )
)

Seems like everything should work now, right?

Nope, because $add_dep within a ParamSet calls:

map_lgl(cond$rhs, self$params[[on]]$test)

to test whether the right hand side of the condition is actually valid for the parameter type.

But mlr3misc::map_lgl internally will call vapply and vapply does not work for functions like expected but only for vectors and objects, i.e.:

vapply(identity, is.function, logical(1))

yields

   x       
FALSE FALSE

but of course is.function(identity) yields TRUE, so the param check within $add_dep "should" pass. All in all I guess this is just too many edge cases coming together.

@mb706
Copy link
Contributor

mb706 commented Jun 21, 2020

I don't know if this is that useful. Dependencies on the "Learner"-side are currently pretty broken and I don't think they should be used in PipeOps. The "tuner" side otoh only has atomic values so this is not a problem there.

@berndbischl
Copy link
Sponsor Member

Dependencies on the "Learner"-side are currently pretty broken

summarizing this as "broken" really seems not appropriate

@berndbischl
Copy link
Sponsor Member

I don't think they should be used in PipeOps

and saying "they should not be used" also sounds weird?

@berndbischl
Copy link
Sponsor Member

also, if you read the code that leads to this bug, it simply comes from the fact that the code does not respect the type of "rhs".
gblame says it was changed by @jakob-r.
the point is that indexing and mapping operations are applied to Cond$rhs, and nowhere in the code it is guaranteed that rhs is a vector / list

@berndbischl
Copy link
Sponsor Member

so,

ps$add_dep("test", on = "fun", cond = CondEqual$new(list(identity)))

actually works (that this is coded wrongly on the inside is still bad!)

@berndbischl
Copy link
Sponsor Member

its not that simple. the whole Condition code becomes kinda ugly if we depend on Uty Params. OTOH we never assert that we do NOT ally UTY.

I created a branch here, but I am not sure what to do best in terms of specs whether stuff CAN depend on UTY or not.

https://github.com/mlr-org/paradox/tree/fix_278_condeq_and_funs

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

No branches or pull requests

4 participants