R-package #208
R-package #208
Conversation
|
@StrikerRUS now that I've added the git sub module any suggestions? I can't extract any info from the #166 on what should be done. |
|
So, git created submodule automatically?.. You've just copy-pasted your repo in the new folder, right? |
|
yes that's correct I just copy-pasted the repo. Detailed : I did the fork, then clone locally, I copy-pasted my repo to the fork and I pushed the fork to my Github account, then I did the pull request. |
|
As you can see this resulted in creating the submodule: https://github.com/RGF-team/rgf/pull/208/files I didn't expect this... Please try the following: navigate to the |
|
@StrikerRUS any idea how this can be done. I removed the .git folder locally then I pushed the changes however I receive, On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean
|
|
Hmm... What gives Maybe this can help?
UPD: Also, please remove |
|
I managed to add |
|
I've fixed this PR, so we can start review now. :-) @mlampros |
R-package/DESCRIPTION
Outdated
| Date: 2018-07-22 | ||
| Authors@R: c( person("Lampros", "Mouselimis", email = "mouselimislampros@gmail.com", role = c("aut", "cre")), person("Ryosuke", "Fukatani", role = "cph", comment = "Author of the python wrapper of the 'Regularized Greedy Forest' machine learning algorithm"), person("Tong", "Zhang", role = "cph", comment = "Author of the 'Regularized Greedy Forest' and of the Multi-core implementation of Regularized Greedy Forest machine learning algorithm"), person("Rie", "Johnson", role = "cph", comment = "Author of the 'Regularized Greedy Forest' machine learning algorithm") ) | ||
| Maintainer: Lampros Mouselimis <mouselimislampros@gmail.com> | ||
| BugReports: https://github.com/mlampros/RGF/issues |
There was a problem hiding this comment.
Please update the link with https://github.com/RGF-team/rgf/issues
There was a problem hiding this comment.
fixed and pushed changes
R-package/DESCRIPTION
Outdated
| Authors@R: c( person("Lampros", "Mouselimis", email = "mouselimislampros@gmail.com", role = c("aut", "cre")), person("Ryosuke", "Fukatani", role = "cph", comment = "Author of the python wrapper of the 'Regularized Greedy Forest' machine learning algorithm"), person("Tong", "Zhang", role = "cph", comment = "Author of the 'Regularized Greedy Forest' and of the Multi-core implementation of Regularized Greedy Forest machine learning algorithm"), person("Rie", "Johnson", role = "cph", comment = "Author of the 'Regularized Greedy Forest' machine learning algorithm") ) | ||
| Maintainer: Lampros Mouselimis <mouselimislampros@gmail.com> | ||
| BugReports: https://github.com/mlampros/RGF/issues | ||
| URL: https://github.com/mlampros/RGF |
There was a problem hiding this comment.
https://github.com/RGF-team/rgf/tree/master/R-package
There was a problem hiding this comment.
fixed and pushed changes
R-package/DESCRIPTION
Outdated
| Maintainer: Lampros Mouselimis <mouselimislampros@gmail.com> | ||
| BugReports: https://github.com/mlampros/RGF/issues | ||
| URL: https://github.com/mlampros/RGF | ||
| Description: Regularized Greedy Forest wrapper of the 'Regularized Greedy Forest' <https://github.com/fukatani/rgf_python> 'python' package, which also includes a Multi-core implementation (FastRGF) <https://github.com/baidu/fast_rgf>. |
There was a problem hiding this comment.
fixed and pushed changes
R-package/DESCRIPTION
Outdated
| URL: https://github.com/mlampros/RGF | ||
| Description: Regularized Greedy Forest wrapper of the 'Regularized Greedy Forest' <https://github.com/fukatani/rgf_python> 'python' package, which also includes a Multi-core implementation (FastRGF) <https://github.com/baidu/fast_rgf>. | ||
| License: MIT + file LICENSE | ||
| SystemRequirements: Python (2.7 or >= 3.4), rgf_python, scikit-learn (>= 0.18.0), scipy ( >= 1.0.0), numpy ( >= 1.14.0). Detailed installation instructions for each operating system can be found in the README file. |
There was a problem hiding this comment.
scipy ( >= 1.0.0), numpy ( >= 1.14.0).
Can you please explain this?
There was a problem hiding this comment.
The RGF R package includes the mat_2scipy_sparse and TO_scipy_sparse functions. For these two functions to work properly, scipy and numpy are the requirements.
There was a problem hiding this comment.
Yeah, sure! rgf_python requires these packages too, because scikit-learn requires them =)
I'm wondering about such strange (very fresh) versions.
R-package/DESCRIPTION
Outdated
| VignetteBuilder: knitr | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Please remove 3 excess blank lines in the end of the file
There was a problem hiding this comment.
fixed and pushed changes
|
@StrikerRUS so that I'm sure that I updated the correct DESCRIPTION file. Inside the R-package folder now appears an RGF folder. I updated the DESCRIPTION file of the RGF folder and not the DESCRIPTION file of the R-package folder (it's my first pull request and I'm sorry if this is obvious) |
|
@mlampros No problem! Did you do the following before committing?
|
|
@StrikerRUS I did the following: cd /rgf-1 # this is the fork of rgf Here I fixed the DESCRIPTION file as you suggested and then, git add . |
|
Strange... Lets do the following. I'll revert your last commit with duplicated files in wrong folder. After that you'll remove and re-clone the |
|
yes, you can proceed. Notify me once I have to re-fork the rgf repository. |
This reverts commit 561e9e7.
You don't need to re-fork, just re-clone your repository ( |
R-package/R/zzz.R
Outdated
|
|
||
| .onAttach <- function(libname, pkgname) { | ||
|
|
||
| packageStartupMessage("Begninning from version 1.0.3 the 'dgCMatrix_2scipy_sparse' function was renamed to 'TO_scipy_sparse' and now accepts either a 'dgCMatrix' or a 'dgRMatrix' as input. The appropriate format for the 'RGF' package in case of sparse matrices is the 'dgCMatrix' format (scipy.sparse.csc_matrix)") |
R-package/README.md
Outdated
|
|
||
|
|
||
|
|
||
| #### **Macintosh OSX** [ installed / tested on Python >= 3.4 ] |
|
|
||
| testthat::expect_true( length(pr) == length(y_reg) && sum(validate) == 15 && is.double(tmp_score) && is.double(tmp_score_W) ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
We need irregular arguments test. ex. n_estimators < 0.
It is not necessary to examine all the arguments, but it is better for one argument per estimator class to have such a test.
There was a problem hiding this comment.
You are right. However, since the R-package ports the Python-package I don't think it's wise to repeat the same tests.
There was a problem hiding this comment.
OK, you don't have to test rgf_python here.
But what happen in R layer if rgf_python raised exception?
reticulate defines behavior in such case?
If so, we don't have to test reticulate function.
There was a problem hiding this comment.
I think an example can solve the issue here,
library(RGF)
set.seed(1)
x = matrix(runif(1000), nrow = 100, ncol = 10)
y = runif(100)
RGF_regr = RGF_Regressor$new(max_leaf = -1)
RGF_regr$fit(x, y)
This is the error that I receive using the Rstudio IDE,
Show Traceback
Rerun with Debug
Error in py_call_impl(callable, dots$args, dots$keywords) :
ValueError: max_leaf must be greater than 0 but was -1.
Concerning the tests of the *reticulate* package, if you don't mind I would like to keep the tests intact, because I have also to submit the package to CRAN before I update the version for the RGF-team repository and this might lead to issues.
|
All problems with certificates have been solved. arxiv.org certificate has been added to the rest certificates, link to r-pkg.org has been replaced with link to the package official page at CRAN, link to the MinGW-w64 title page has been replaced with http protocol, link to MinGW-w64 download page has been replaced with the direct link to SourceForge. @mlampros Please fix the WARNING which has been introduced in 4470326 |
|
|
||
| else { | ||
|
|
||
| stop("the function can take either a 'sparse_row_matrix' or a 'sparse_column_matrix' for the 'format' parameter as input", call. = F) |
There was a problem hiding this comment.
yes, it is tested
|
|
||
| <br><br> | ||
|
|
||
| #### **Windows OS** [ installed / tested on Python >= 3.4 ] |
There was a problem hiding this comment.
(In the future, we should test by appveyor.)
There was a problem hiding this comment.
I don't know if I can help for this particular matter, but I think in case of Windows the integration of a docker image is necessary (I haven't done this before)
There was a problem hiding this comment.
Yeah, lets do this in next PR, not this one.
| exit -1 | ||
| #elif grep -q -R "NOTE" "$LOG_FILE_NAME"; then | ||
| # echo "NOTES have been found in the build log!" | ||
| # exit -1 |
There was a problem hiding this comment.
If unnecessary, l.55-l.57 should be deleted.
There was a problem hiding this comment.
I guess @StrikerRUS should decide if this code chunk should be removed
There was a problem hiding this comment.
These lines will be uncommented right after the merging this PR. The reason is that at present there is no rgf/R-package directory and CRAN check produces a NOTE for this.
"update the R-package files"
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
=========================================
Coverage ? 16.66%
=========================================
Files ? 3
Lines ? 60
Branches ? 0
=========================================
Hits ? 10
Misses ? 50
Partials ? 0Continue to review full report at Codecov.
|
|
Does anyone know why codecov appeared here?? Before now it was silent and I liked it. |
|
I have never used codecov. Has anyone registered? Otherwise is it a codecov bug? |
| # exit -1 | ||
| fi | ||
|
|
||
| Rscript -e 'covr::codecov(quiet = FALSE)' |
There was a problem hiding this comment.
This line can be related to codecov behavior.
There was a problem hiding this comment.
This line has been here since the first commit in the PR. Before today it silently printed information to the Travis log and updated the badge, but today decided to come here in comments and post the report.
PS. it seems that registration at codecov.io isn't required and done automatically after using the package.
There was a problem hiding this comment.
I want to rollback to the previous behavior: report is printed to the Travis log and badge updates without these useless and annoying comments.
There was a problem hiding this comment.
I've found the way to easily disable comments, but this PR should be merged first.
|
I have tested different versions of R. The results are following. 3.5.1 (current):OK 3.4.4:3.4.0:3.3.0:3.2.3:3.2.0:3.1.0:3.0.0:I don't know what's the license note mean, but I think that we can easily downgrade the required R version to 3.0.0 (WARNING is a bug of the test environment, not the package itself). @mlampros What was the reason of setting min version to 3.2.3? |
|
@StrikerRUS the main reason that I use the 3.2.3 version is because I don't want to force users to use the latest version (currently 3.5.1) but on the other hand to avoid using a too old version, as new features and bugs are introduced / fixed from one version to another. You can have a look to the news sections. |
|
@mlampros Sorry, didn't get it. I'm speaking about earlier versions. On what concrete features/bug fixes of 3.2.3 version RGF is relied? OK, I'll paraphrase. Imagine, I'm using R 3.2.0 and you are forcing me to upgrade it to 3.2.3. Why? The fact is that RGF can work with my version of R. You cannot set min version out of nothing. |
|
@StrikerRUS, every time I have to test-check any package that I maintain I've to test it with the latest R-devel version. I guess the 3.2.3 version is for me, as I said, not the newest but also not the oldest one. However, the RGF package imports the following packages,
You are probably right on that I've have to update the minimum version to 3.2.0, however I didn't like the tone of your comment. |
Sorry, I didn't want to hurt you! Probably it's due to my non-perfect English :-) Speaking about this,
it's true only for the two latest (1.2-13, 1.2-14) versions of the Matrix package. Is there any reason to force users use them? Because 1.2-12 version, which was published in November 2017 (I think rather fresh), requires R >= 3.0.1: https://github.com/cran/Matrix/blob/24289cf538a36e90ad4bf8041f51d3397559fd9e/DESCRIPTION |
|
Thanks to huge contribution by @mlampros and @StrikerRUS , It seems that this PR has quality that can be merged in almost. In my opinion, R version is still as it is, and expansion of support can be discussed in another PR. |
|
@fukatani Yeah! Absolutely agree! My browser even freezes when opens this PR discussion - so many comments 😄 I'll create separate issues for some comments, which left undiscussed here. |
This pull request is the continuation of the #165 issue.