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

Fail earlier in case of multiple levelplots #39

Merged
merged 1 commit into from Jul 6, 2016

Conversation

@mvkorpel
Copy link
Contributor

@mvkorpel mvkorpel commented Jul 4, 2016

The function plot.mboost() does not allow drawing multiple levelplots. Therefore there is no point in creating and storing (in RET) more than one such plot. I'm suggesting this patch which may save a lot of resources (processing time and memory) by not calling levelplot() more than once.

@coveralls
Copy link

@coveralls coveralls commented Jul 4, 2016

Coverage Status

Coverage increased (+0.009%) to 79.484% when pulling df73cc7 on mvkorpel:one-levelplot into b893002 on boost-R:master.

@hofnerb
Copy link
Member

@hofnerb hofnerb commented Jul 4, 2016

Good point and valid solution.

However, one could also return all levelplots and subsequently plot the list by hand or by explicitly calling print() on each level plot at the end of the function. Any thoughts?

@mvkorpel
Copy link
Contributor Author

@mvkorpel mvkorpel commented Jul 4, 2016

I don't have a strong opinion on how multiple levelplots should be dealt with. It appears that the current policy of no more than one levelplot was introduced in commit 7e7ef57, release: mboost 2.1-0. I don't know the reasoning behind the change. The objects returned by levelplot() can be large so maybe that's got something to do with the limitation, but the plot objects are stored anyway before throwing the error (before my suggested patch), so there goes that argument... Maybe there could be one or more parameters that let the user decide if (multiple) levelplots should be returned (for later customization or layout adjustments) and/or drawn immediately.

@hofnerb hofnerb merged commit 358ac53 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.009%) to 79.484%
Details
@mvkorpel mvkorpel deleted the mvkorpel:one-levelplot branch Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.