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

fix minor typos in READMEs #239

Merged
merged 1 commit into from
Sep 2, 2018
Merged

fix minor typos in READMEs #239

merged 1 commit into from
Sep 2, 2018

Conversation

jameslamb
Copy link
Contributor

I learned about RGF this morning and was poking around the documentation. Please consider this PR to fix minor typos.

@mlampros
Copy link
Collaborator

mlampros commented Sep 1, 2018

@jameslamb,

the first typo is in the R package, so thanks (I recently added the RGF R package in the RGF-team repository). If I may ask, in which OS did you install the RGF R package?

@jameslamb
Copy link
Contributor Author

@mlampros I haven't actually installed it yet, was just reading through doc.

I am going to try installing on MacOS later today.

@jameslamb
Copy link
Contributor Author

Do you have an open issues you want addressed in the R package? I don't see any here but would be happy to help!

FYI I found this project from @StrikerRUS 's profile

@StrikerRUS
Copy link
Member

StrikerRUS commented Sep 1, 2018

Happy to see you here, James! And thank you for the fixing typos.

Will merge this PR right after #238 (I like green ticks of CI checks 😄 ). BTW, that PR is a hotfix for the R-package test. Seems that there are many issues with default libcurl at Travis which is used behind the R CMD check --as-cran command.

Since we've started to talk about the R-package, may I ask you guys to increase the coverage of the R code? codecov.io

@mlampros
Copy link
Collaborator

mlampros commented Sep 1, 2018

@jameslamb the RGF R package was initially hosted in my account. In the next version of RGF, which I'll submit probably this or next week, the repo will be archived and the past issues as well. The only open issue currently is the fact that the RGF R package can be run on windows only from within the command line and not from an R gui (such as Rstudio). I'll have to open an issue in this repository with a feature request or help needed badge, once I submit the new version to CRAN. I found that the installation on Windows was somehow cumbersome that's why I also created a singularity image.

@StrikerRUS
Copy link
Member

@jameslamb Please rebase to master to pass CI check.

@jameslamb
Copy link
Contributor Author

rebased and pushed! should build now

@jameslamb
Copy link
Contributor Author

@StrikerRUS to answer your question...yep I can try to improve the coverage! Will try to get a build going on my Mac, then I'll go from there. 73% is a pretty good start!

@StrikerRUS
Copy link
Member

@jameslamb Thank you very much!

@mlampros
Copy link
Collaborator

mlampros commented Sep 2, 2018

@jameslamb, @StrikerRUS there are actually 3 files in the RGF R package. The package.R file is used mainly to load the python packages (based on the reticulate package), so the coverage can not be improved. The zzz.R has already 100% coverage and the utils.R has 82.7 % coverage. In the utils.R file I've added two new methods in the last version that should be tested and two more cases is in the mat_2scipy_sparse method.

@jameslamb
Copy link
Contributor Author

@mlampros I noticed that the coverage I got on utils.R locally was less (the 73% mentioned by @StrikerRUS )

What is the purpose of these guards against testing:

skip_test_if_no_module <- function(MODULE) {

  if (length(MODULE) == 1) {

    module_exists <- reticulate::py_module_available(MODULE)}

  else {

    module_exists <- sum(as.vector(sapply(MODULE, function(x) reticulate::py_module_available(x)))) == length(MODULE)
  }

  if (!module_exists) {

    testthat::skip(paste0(MODULE, " is not available for testthat-testing"))
  }
}

^ I suspect that some tests were not getting run on my machine because I didn't correctly install the Python package. Shouldn't that just make them break?

@StrikerRUS
Copy link
Member

StrikerRUS commented Sep 2, 2018

@jameslamb Approximately 17%? Then yes, you have broken package. I had the same problem when tried to create test environment at Travis. #230 (comment)

I've already asked about these stubs #208 (comment)...

@mlampros
Copy link
Collaborator

mlampros commented Sep 2, 2018

@jameslamb once you install the RGF R package, I would suggest to use the covr package and especially the covr::package_coverage() to come to the current package coverage.

The skip_test_if_no_module function is necessary so that the package passes the CRAN tests. An explanation can be found in the documentation of the reticulate package

@StrikerRUS StrikerRUS merged commit 6e31336 into RGF-team:master Sep 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants