Skip to content

/ mboost

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

Jump to bottom

Improved PR #42

Merged
merged 4 commits into from Jul 6, 2016
Merged

Improved PR #42

merged 4 commits into from Jul 6, 2016

Conversation

@hofnerb
Copy link
Member

@hofnerb hofnerb commented Jul 6, 2016

No description provided.

mvkorpel and others added 4 commits Jul 6, 2016
@mvkorpel
Better comparison of mclapply and function given as argument
692c293
@mvkorpel
Use match.fun(): 'papply' can be the name of a function
be41184
@mvkorpel
More robust use of all.equal() in if clause
Loading status checks…
98a39b4
@hofnerb
Update crossvalidation.R
Loading status checks…
251bca0 
No need to introduce new argument/function name
@hofnerb hofnerb changed the title Patch 1 Improved PR Jul 6, 2016
@coveralls
Copy link

@coveralls coveralls commented Jul 6, 2016

Coverage Status

Coverage increased (+0.004%) to 79.479% when pulling 251bca0 on hofnerb:patch-1 into 77ac6a2 on boost-R:master.
@mvkorpel
Copy link
Contributor

@mvkorpel mvkorpel commented Jul 6, 2016

Yes, there is no need to introduce a new variable to hold the matched function. It is just a personal habit of mine not to modify the arguments of a function. This is one of the things checked by codetools::checkUsagePackage("mboost", all = TRUE). Using the same name for an updated value is not a problem in this case, but may be in others, due to lazy evaluation employed by R. It is perfectly fine to modify an argument in many (most?) cases, and that is probably why the corresponding codetools check is not used by default, hence the all = TRUE.
@hofnerb hofnerb merged commit 7e252a2 into boost-R:master Jul 6, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 79.479%
Details
@hofnerb hofnerb deleted the hofnerb:patch-1 branch Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@hofnerb @coveralls @mvkorpel
You can’t perform that action at this time.