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

add trafo parameter for discretevector #212

Merged
merged 4 commits into from
Apr 8, 2019
Merged

Conversation

rcannood
Copy link
Contributor

@rcannood rcannood commented Mar 21, 2019

Would it be possible to add the trafo function for the discretevectorparam?

[Edit: the proposed changes happened in commit 92da645. The other commits are small improvements to unit tests and documentation and might cause confusion when looking at "Files changed".]

It's absolutely vital for a function in a package I wish to upload to CRAN.

I ran a R CMD check and a revdepcheck, and they both showed that this change would not break any downstream packages.

R CMD check

── R CMD check results ──────────── ParamHelpers 1.13 ────
Duration: 3m 2.6s

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

R CMD check succeeded

revdepcheck

── CHECK ────────────────────────────────── 16 packages ──
✔ CEGO 2.3.0                             ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ CEGO 2.3.0                             ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ llama 0.9.2                            ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ aslib 0.1                              ── E: 1     | W: 0     | N: 0                                                                                                                                            
✔ metagen 1.0                            ── E: 1     | W: 0     | N: 0                                                                                                                                            
✔ cmaesr 1.0.3                           ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ ecr 2.1.0                              ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ flacco 1.7                             ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ OpenML 1.8                             ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ bnclassify 0.4.2                       ── E: 0     | W: 2     | N: 3                                                                                                                                            
✔ randomsearch 0.2.0                     ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ mlrCPO 0.3.4-2                         ── E: 0     | W: 0     | N: 1                                                                                                                                            
✔ SIAMCAT 1.0.0                          ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ smoof 1.5.1                            ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ tuneRanger 0.4                         ── E: 0     | W: 0     | N: 0                                                                                                                                            
✔ mlrMBO 1.1.2                           ── E: 0     | W: 0     | N: 1                                                                                                                                            
✔ mlr 2.13                               ── E: 0     | W: 0     | N: 2                                                                                                                                            
OK:                                                                                                                                                                                                             
BROKEN: 0
Total time: 49 min

@coveralls
Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage increased (+0.1%) to 92.188% when pulling b9b8846 on rcannood:master into 649d128 on berndbischl:master.

@rcannood
Copy link
Contributor Author

I improved one of the unit tests a little bit, as a way of thanking you for adding the trafo parameter :)

@jakob-r
Copy link
Member

jakob-r commented Mar 25, 2019

I improved one of the unit tests a little bit, as a way of thanking you for adding the trafo parameter :)

Okay, I got confused, why these changes were made. Could you add a test that also uses trafos on discrete vectors?

@rcannood
Copy link
Contributor Author

Sorry for the confusion :)

It makes sense that I would also add a test for checking whether the trafo works on discrete vectors; this has been added in b9b8846.

@jakob-r
Copy link
Member

jakob-r commented Mar 26, 2019

As far as I'm concerned, it can be merged. @berndbischl?

@rcannood
Copy link
Contributor Author

🙇‍♂️

@jakob-r jakob-r merged commit 8916912 into mlr-org:master Apr 8, 2019
@jakob-r
Copy link
Member

jakob-r commented Apr 8, 2019

Merged

@rcannood
Copy link
Contributor Author

rcannood commented Apr 8, 2019

Thanks a lot!

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