-
Notifications
You must be signed in to change notification settings - Fork 57
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
increase code coverage for the R-package #313
Conversation
@mlampros Thank you very much for this PR! Seems that two of new tests don't work on macOS:
|
@StrikerRUS, I assumed that Mac osx will have the same temporary folder as on linux ('/tmp'). |
@mlampros I guess you can use |
@StrikerRUS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlampros Thank you! Please check some my comments about tests below.
@mlampros Sorry for late response. Are you debugging failure on appveyor? |
@fukatani, I'll add the remaining tests for this PR tonight. I think, the appveyor error is a connection failure, so with a re-run it will disappear. thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlampros Thank you very much for your hard work! I left some minor comments below.
A Fast Regularized Greedy Forest classifier | ||
|
||
A Fast Regularized Greedy Forest classifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these duplicated lines expected or it is a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. The previous .Rd file consisted of a single line. The .Rd file is the manual page of the corresponding .R file. It has to do with the roxygen2 package and how it builds the manual pages from the .R files (I've used the roxygen2 7.0.2 version for this PR)
\description{ | ||
Regularized Greedy Forest regressor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous .Rd file consisted of a single line. The .Rd file is the manual page of the corresponding .R file. It has to do with the roxygen2 package and how it builds the manual pages from the .R files (I've used the roxygen2 7.0.2 version for this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM excepts above the comment.
I've added code coverage as discussed in #269. The only exception is the cleanup() method (either for a single estimator or for all estimators) which is tested (currently) on a unix-like OS. Feel free to close #269 if this PR works as expected.
One think that I've noticed is that the 'save_model' and 'dump_model' methods work only for RGF and not for FastRGF. thanks.