liquidSVM #2428
liquidSVM #2428
Conversation
|
Could you add some tests please? |
|
Why are there some errors on the pr and not on the push? I also do not understand the errors very well... |
|
Ok, it looks like the tests are failing with a segmentation fault. I've tried on two different machines and can't reproduce it. Does it work when you run the tests locally? |
|
Yes, they were ok. The algorithm only produces warnings in the default version which is not good. Also the probability prediction is not working properly yet. But the package has quite potential regarding performance and runtime and I already contacted the creators. |
R/RLearner_classif_liquidSVM.R
Outdated
| package = "liquidSVM", | ||
| par.set = makeParamSet( | ||
| makeIntegerLearnerParam(id = "d", lower = 0L), | ||
| makeLogicalLearnerParam(id = "scale"), |
R/RLearner_classif_liquidSVM.R
Outdated
| cl = "classif.liquidSVM", | ||
| package = "liquidSVM", | ||
| par.set = makeParamSet( | ||
| makeIntegerLearnerParam(id = "d", lower = 0L), |
There was a problem hiding this comment.
This should not be tunable (tunable = FALSE).
R/RLearner_classif_liquidSVM.R
Outdated
| par.set = makeParamSet( | ||
| makeIntegerLearnerParam(id = "d", lower = 0L), | ||
| makeLogicalLearnerParam(id = "scale"), | ||
| makeIntegerLearnerParam(id = "threads", lower = -1L), |
R/RLearner_classif_liquidSVM.R
Outdated
| makeIntegerLearnerParam(id = "d", lower = 0L), | ||
| makeLogicalLearnerParam(id = "scale"), | ||
| makeIntegerLearnerParam(id = "threads", lower = -1L), | ||
| makeIntegerLearnerParam(id = "partition_choice", lower = 0L), |
| makeLogicalLearnerParam(id = "scale"), | ||
| makeIntegerLearnerParam(id = "threads", lower = -1L), | ||
| makeIntegerLearnerParam(id = "partition_choice", lower = 0L), | ||
| makeIntegerLearnerParam(id = "grid_choice", lower = -2L, upper = 2L), |
There was a problem hiding this comment.
How does this interact with gamma_steps, lambda_steps, etc? Are these options mutually exclusive?
There was a problem hiding this comment.
And all of these options should be not tunable.
R/RLearner_classif_liquidSVM.R
Outdated
| makeIntegerLearnerParam(id = "partition_choice", lower = 0L), | ||
| makeIntegerLearnerParam(id = "grid_choice", lower = -2L, upper = 2L), | ||
| makeIntegerLearnerParam(id = "adaptivity_control", lower = 0L, upper = 2L), | ||
| makeIntegerLearnerParam(id = "random_seed", default = 1), |
There was a problem hiding this comment.
I didn't find any documentation on what the default random seed is -- where did you find this? It also shouldn't be tunable.
R/RLearner_classif_liquidSVM.R
Outdated
| makeIntegerLearnerParam(id = "grid_choice", lower = -2L, upper = 2L), | ||
| makeIntegerLearnerParam(id = "adaptivity_control", lower = 0L, upper = 2L), | ||
| makeIntegerLearnerParam(id = "random_seed", default = 1), | ||
| makeIntegerLearnerParam(id = "fold"), |
There was a problem hiding this comment.
Should that be folds? And there should be a minimum for this (and again not tunable).
R/RLearner_classif_liquidSVM.R
Outdated
| makeIntegerLearnerParam(id = "adaptivity_control", lower = 0L, upper = 2L), | ||
| makeIntegerLearnerParam(id = "random_seed", default = 1), | ||
| makeIntegerLearnerParam(id = "fold"), | ||
| makeIntegerLearnerParam(id = "clipping"), |
There was a problem hiding this comment.
Seems that the minimum is -1. Also probably shouldn't be tunable.
R/RLearner_classif_liquidSVM.R
Outdated
| makeNumericLearnerParam(id = "max_lambda", lower = 0), | ||
| makeNumericVectorLearnerParam(id = "lambdas", lower = 0), | ||
| makeNumericVectorLearnerParam(id = "c_values", lower = 0), | ||
| makeLogicalLearnerParam(id = "useCells") |
There was a problem hiding this comment.
Seems that the default for this is FALSE. Should be not tunable.
|
I tried to change all parameters according to your suggestions. I am unsure about clipping, if it is tunable or not, that's why I did not set |
|
Thanks, looks good -- what about restrictions between parameters that control the tuning though? If you set all the gamma parameters at the same time, I guess that some of them override others? It may make sense to add requirements saying that if you set a certain parameter, others cannot be set to avoid surprising results when users accidentally set conflicting parameters. |
I changed the parameter settings to take into account requirements. Moreover I looked into https://github.com/cran/liquidSVM/blob/master/R/mlr.R and changed the parameter settings slightly. |
|
Thanks for all your work on this. Merging. |
* create liquidSVM learners * add the necessary package * include some tests (that are still failing (probably)) * add debug seed * change parameter settings of liquidSVM
2 participants
This is the implementation of liquidSVM as mlr-learner. The probability predictions are still technically flawed and they are not (yet) responding to my emails, so unfortunately probability predictions are not provided at the moment.