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

Fixes for some issues #183

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fixes for some issues #183

wants to merge 11 commits into from

Conversation

mb706
Copy link
Collaborator

@mb706 mb706 commented Jun 25, 2017

Fixes for: #180 #181 #182

@coveralls
Copy link

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.03%) to 96.03% when pulling 8096d71 on mb706:mb706_fixes into e233aa7 on berndbischl:master.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-0.03%) to 96.03% when pulling b98b927 on mb706:mb706_fixes into e233aa7 on berndbischl:master.

@@ -65,6 +65,6 @@ discreteValueToName = function(par, x) {
if (par$type == "discrete") {
ns[getIndex(par$values, x)]
} else if (par$type == "discretevector") {
sapply(x, function(x) ns[getIndex(x, values = par$values)])
vcapply(x, function(app x) ns[getIndex(x, values = par$values)])
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nice thing about sapply is that it gives list() in case of length(x) == 0, which would match quite nicely with the actual default being list(). When the related BBmisc issue gets fixed, this will result in the most natural printout. vcapply will give character(0) when x is list(), which is a deviation from the behaviour of the other "vector" types (which always show logical(0), numeric(0) etc. depending on their type).

Not going to say much about the syntax error here.

Copy link
Member

@jakob-r jakob-r Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the typo. But in the documentation we say that the output is a character vector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I didn't consider the other points from which discreteValueToName gets called. I'd still say the printout of a discreteVectorLearnerParam with default list() shouldn't have character(0) in it.

@mb706
Copy link
Collaborator Author

mb706 commented Jun 26, 2017

My comment still stands

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-0.03%) to 96.03% when pulling c19ca1f on mb706:mb706_fixes into e233aa7 on berndbischl:master.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.2%) to 96.268% when pulling a3d2c6a on mb706:mb706_fixes into e233aa7 on berndbischl:master.

@jakob-r
Copy link
Member

jakob-r commented Jun 28, 2017

Because this thread is invisible let's continue it here:

@mb706 wrote: You're right, I didn't consider the other points from which discreteValueToName gets called. I'd still say the printout of a discreteVectorLearnerParam with default list() shouldn't have character(0) in it.

I agree in this point. You mentioned it here but the conversion to character(0) is already here (discreteValueToName). But it could also be fixed in paramValueToString.Param.

@mb706
Copy link
Collaborator Author

mb706 commented Jul 25, 2017

> makeDiscreteVectorLearnerParam("x", NA, values=c("a", "b", "c"), default = list())
            Type len    Def Constr Req Tunable Trafo
1 discretevector  NA list()  a,b,c   -    TRUE     -
Used: train.

looks nice.

@jakob-r jakob-r added this to the v1.12 milestone Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants