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.

Sign up for GitHub

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

Rpolyhedra #157

Closed
10 of 15 tasks
kenarab opened this issue Nov 9, 2017 · 43 comments
Closed
10 of 15 tasks

Rpolyhedra #157

kenarab opened this issue Nov 9, 2017 · 43 comments
Assignees
@noamross
Labels
6/approved package topic:mathematics

Comments

@kenarab
Copy link

kenarab commented Nov 9, 2017 •
edited by noamross
Loading

Submitting Author: Alejandro Baranek" (@kenarab)
Repository: https://github.com/qbotics/Rpolyhedra
Version submitted: 0.1.0
Editor: @noamross
Reviewer 1: @yulijia
Reviewer 2: @schloerke
Archive: N/A
Version accepted: 0.4.1

Summary

  • What does this package do? (explain in 50 words or less):

A 142 polyhedra Database scraped from PHD as R6 objects and RGL visualizing capabilities. The PHD format was created to describe the geometric polyhedron definitions derived mathematically by Andrew Hume and by the Kaleido program of Zvi Har'El.

  • Paste the full DESCRIPTION file inside a code block below:
Package: Rpolyhedra
Type: Package
Title: Polyhedra Database
Version: 0.1.0
Authors@R: c(
    person("Alejandro", "Baranek",,"abaranek@dc.uba.ar", c("aut","com","cre", "cph")),
    person("Leonardo","Belen",,"leobelen@gmail.com", c("aut","com","cph"))
    )
Maintainer: Alejandro Baranek <abaranek@dc.uba.ar>
Description: A 142 polyhedra database scraped from PHD files <http://paulbourke.net/dataformats/phd/> as R6 objects and 'rgl' visualizing capabilities. The PHD format was created to describe the geometric polyhedra definitions derived mathematically <http://www.netlib.org/polyhedra/> by Andrew Hume and by the Kaleido program of Zvi Har'El.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1
Suggests: knitr, rmarkdown,
    covr
VignetteBuilder: knitr
Depends: R (>= 3.0.0)
Imports: futile.logger,
         rgl,
         stringr,
         R6,
         testthat
Collate:
  'polyhedra-lib.R'
  'zzz.R'
BugReports: https://github.com/qbotics/Rpolyhedra/issues

  • URL for the package (the development repository, not a stylized html page):
    https://github.com/qbotics/Rpolyhedra

  • Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

[e.g., "data extraction, because the package parses a scientific data file format"]

data retrieval, because includes the code for downloading the phd files
data extraction, because the package parses a scientific data file format
database access, because it gives access to PHD polyhedra database

Other categories:
legacy sourcecode: as poly is an opensource inciative (from '80s) than no longer compiles, the project gives access to original poly outputs.

  • Who is the target audience?

    • Scientists and general public who wants to explore polyhedra through RGL.
    • Scientists and general public who wants to study polyhedra's properties and make taxonomies
    • Scientists and general public who wants to make models over polyhedra definitions

  • Are there other R packages that accomplish the same thing? If so, how does
    yours differ or meet our criteria for best-in-category?

RGL includes 5 regular solids. Rpolyhedra provides version of the 5 regular solids and 137 more polyhedra.

Requirements

Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coeveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)

Detail

  • Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:

  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

  • If this is a resubmission following rejection, please explain the change in circumstances:

  • If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:

  • Delete the file inst/extdata/polyhedra.RDS for scraping and regenerating it.
  • Compile the package with a small number of polyhedra first:
.polyhedra <- scrapePolyhedra(max.quant = 5, test = FALSE)
  • We have to find a way for accelerate the scraping of the whole database. Maybe a reviewer know a tweak.
@sckott sckott added the package label Nov 9, 2017
@sckott
Copy link
Contributor

sckott commented Nov 9, 2017

thanks for your submission @kenarab

Please change the URL to https://github.com/qbotics/Rpolyhedra/ instead of https://github.com/cran/Rpolyhedra/

We need some clarification to assess fit

  • What are the scientific uses of these data? You say scientists are a target audience, but what kind? What would they do with these data?
  • I know little about this area. Are there alternative ways to specify polyhedra? If so, what is most widely used?

@kenarab
Copy link
Author

kenarab commented Nov 10, 2017 •
edited
Loading

Thank you @sckott.

I corrected the URL, and extended the users with this paragraph:

  • Scientists and general public who wants to explore polyhedra through RGL.
  • Scientists and general public who wants to study polyhedra's properties and make taxonomies
  • Scientists and general public who wants to make models over polyhedra definitions

About

  • I know little about this area. Are there alternative ways to specify polyhedra? If so, what is most widely used?

PHD is a legacy open source specification for polyhedra. As we found online that database with solid definition we chose to use it for further research over polyhedra.
For specifying polyhedra solids there is only needed to describe vertex and edges (and the definition is not exclusive). But there are different properties the specialists uses: Schaffi symbols, symmetry/rotation groups, etc.

For example, in Wikipedia there are a lot of information. https://en.wikipedia.org/wiki/Icosahedron
The visualizations of wikipedia are generally made in POV-ray.
There is currently no way for accessing a large database of solid (and net) models of polyhedra in R. That's the target of Rpolyhedra.

Rpolyhedra includes algorithms for triangulating polygon faces for producing RGL Models based only in trangles (tmesh).

We didn't work yetwith polyhedra formulation and generation in Rpolyhedra (Is not really our objective). We only give access to other people work, It's a compilation work.
There are private projects with polyhedra databases but RGL includes only models for 5 regular solids as RGL models. That's why we think Rpolyhedra could be useful for R community.

@sckott
Copy link
Contributor

sckott commented Nov 10, 2017

Thank you @kenarab

@noamross noamross self-assigned this Nov 13, 2017
@sckott
Copy link
Contributor

sckott commented Nov 15, 2017

hi @kenarab thx for your submission - @noamross will be handling editor

@noamross
Copy link
Contributor

noamross commented Nov 16, 2017 •
edited
Loading

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Hi @kenarab, thanks for your submission, I think this is a really neat package. I can think of a few people who would be interested already who I might ask to review. A few things before we can assign reviewers, though:

  • We require that packages have continuous integration (typically Travis-CI), and code coverage reporting (typically codecov or coveralls). Looking at your package tests, I see you are testing the scraping functions but not necessarily what goes on in the R6. We don't require 100% code coverage, but we'd like coverage of the major functionality. As far as I can tell there are three major functionalities: scraping the data from netlib, parsing the PHD format, and providing R6 objects with polyhedral data. All three should have associated tests. You can use skip_on_cran() or skip_on_travis() if long-running scraping calls or RGL calls will break that. Let us know if you'd like some pointers to setting this infrastructure set up.
  • It looks like you have some debugging infrastructure hard-coded into the package right now via the futile.logger calls. This breaks other testing tools and also results in unneeded/confusing messages bring printed out to the user. This should be removed. You might keep it on a development branch.
  • I note that the code doesn't run in your vignette because there isn't a library(rgl) call. You also should consider using the WebGL capability in the rgl package so that you can make a vignette that compiles and shows.
  • Documentation is very sparse - most functions have a single sentence. We try to aim documentation to be sufficient for familiars unfamiliar with the original data source. R6 classes can be a bit difficult to document, but docs should detail the arguments and formats of the member functions. We want documentation to be as complete as possible at submission so reviewers can give feedback on it.

@kenarab
Copy link
Author

kenarab commented Nov 17, 2017 via email

@noamross
Copy link
Contributor

noamross commented Dec 9, 2017

Any update on this, @kenarab?

@kenarab
Copy link
Author

kenarab commented Dec 21, 2017 via email

@noamross
Copy link
Contributor

Hi @kenarab! Just checking to see if you have a time frame for following up on this.

@kenarab
Copy link
Author

kenarab commented Feb 22, 2018 via email

@kenarab
Copy link
Author

kenarab commented Mar 1, 2018 via email

@noamross
Copy link
Contributor

noamross commented Mar 1, 2018

Great, Ale. Let me know when you want me to look again so I can assign reviewers. I'd suggest running goodpractice::gp before you do as that's what we run to check packages before sending out to review.

@kenarab
Copy link
Author

kenarab commented Mar 8, 2018 via email

@kenarab
Copy link
Author

kenarab commented Mar 13, 2018 via email

@kenarab
Copy link
Author

kenarab commented Mar 13, 2018 via email

@kenarab
Copy link
Author

kenarab commented Mar 14, 2018 via email

@noamross
Copy link
Contributor

noamross commented Mar 14, 2018 •
edited
Loading

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Glad we're getting this going again!

You must include a paper.md so our reviewers can review it on behalf of JOSS if you want joint JOSS publication. This does need to happen before reviewers are assigned, but I will go ahead and look for reviewers and have them start once you have the paper in place.

I note that while you have CI set up, you don't have Travis-CI or Codecov badges in your package README. Please add these - they make it easier to see package status at a glance. Also, you may now add the ROpenSci status badge:

[![](https://badges.ropensci.org/157_status.svg)](https://github.com/ropensci/onboarding/issues/157)

It appears the goodpractice::gp() bug is actually in the covr package. For some covr::package_coverage() fails under the the first test, even though your tests pass with flying colors. I suggest you submit a bug report to covr. Otherwise, Here are goodpractice::gp() checks without using the covr checks. Nothing serious here - please fix the small stuff. Reviewers should consider whether changes to code style or package imports are necessary.

Regarding the large file issue: if you have a lot of large polyhedra you would like to serve to users but cannot get on CRAN, may I suggest adding an after-the-fact download function like download_polyhedra(). This is a strategy used by several others; the recently accepted rtika packages uses this approach to deliver a large .jar after installation. Your decision on this can wait until after reviews are in an reviewers have weighed in.

── GP Rpolyhedra ─────────────────────────────────────

It is good practice to

  ✖ add a "URL" field to DESCRIPTION. It helps users find information about your package online. If your package
    does not have a homepage, add an URL to GitHub, or the CRAN package package page.
  ✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80
    characters wide. Try make your lines shorter than 80 characters

    R/db-lib.R:32:1
    R/db-lib.R:67:1
    R/db-lib.R:85:1
    R/db-lib.R:88:1
    R/db-lib.R:109:1
    ... and 134 more lines

  ✖ avoid sapply(), it is not type safe. It might return a vector, or a list, depending on the input data.
    Consider using vapply() instead.

    R/polyhedra-lib.R:142:38

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and 1:NCOL(...) expressions. They are error prone
    and result 1:0 if the expression on the right hand side is zero. Use seq_len() or seq_along() instead.

    R/db-lib.R:435:21
    R/polyhedra-lib.R:162:17
    R/polyhedra-lib.R:300:21
    R/polyhedra-lib.R:473:26
    R/polyhedra-lib.R:477:28
    ... and 2 more lines

  ✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import
    only the specific functions you need.
  ✖ fix this R CMD check NOTE: installed size is 13.6Mb sub-directories of 1Mb or more: extdata 13.4Mb
──────────────────────────────────────────────

Reviewers:
Due date:

@kenarab
Copy link
Author

kenarab commented Mar 14, 2018 via email

@noamross
Copy link
Contributor

@kenarab It took a little longer than I hoped but I have two great reviewers lined up. We can move ahead with starting the review (the GP outputs are minor enough that they can be corrected by the end of the review). I just want to check that you have a stable branch that they can work with.

@kenarab kenarab mentioned this issue Nov 30, 2018
Closed
@kenarab
Copy link
Author

kenarab commented Nov 30, 2018

Hello @noamross. We are finally uploading a new version with all observations you made implemented.

Here is a link to the current release (v0.3.0)

We included a function switchToFullDatabase() for switching to fullDB, which downloads from a separate repository. By default, the packages installs with a minimal db with 120 polyhedra. There are many configurations possible for users but also for development proposes.
All polyhedra is scraped in its own serialized file, so different packing strategies could be implemented in a future, including a scrape-on-demand, for example.

Other interesting feature that worth mentioning is polyhedra RGL models are now modified with transformationMatrix, so common strategies for managing openGL models can be applied.

The rest of modifications are technical, We think V0.3.0 are a good starting point for reviewers and doesn't worth for reviewers to check it out.

Thank you very much for your time and guidance. We hope the package is Ropensci compliant, and we can finish the process to publish within the project.

About JOSS, we didn't upload any paper because we don't think the contribution in this package has to be published in a peer reviewed paper at this point on time.
Best,

Leonardo & Alejandro.

@noamross
Copy link
Contributor

noamross commented Dec 3, 2018

Thank you @kenarab. I see that your overall test coverage is still low at about 30%. Is there a reason for this? Sometimes this occurs because some of the package-building code ends up in the package itself. But coverage less than 75% or so is problematic.

@kenarab
Copy link
Author

kenarab commented Dec 6, 2018 via email

@kenarab
Copy link
Author

kenarab commented Dec 26, 2018 via email •
edited
Loading

@noamross
Copy link
Contributor

noamross commented Dec 29, 2018 •
edited
Loading

Thank you @kenarab . As it's been a while since I originally contacted them I will see whether the original reviewers are still available. Also, a few notes, which I will ask the reviewers to consider. I would recommend not incorporating them until reviews are in, as well, so you can assess the feedback all together:

  • Function documentation is pretty sparse, in that most function descriptions are one-line. However you also include documentation for a large number of non-exported functions that are developer facing. This makes for confusing navigation of help. I'd suggest leaving this documentation only in the source code and not in man pages, which you can do with the @noRd tag. Then focus on expanding the documentation of the smaller number of exported functions. For instance, there is not documentation on the R6 methods available for the Polyhedron class.
  • Similarly, the vignette includes calls to non-exported functions and has little explanatory text. Removing the section on non-exported function to a developer's vignette or appendix that makes it clear these are internal, package-building functions would improve clarity, and the vignette should spend more effort on explaining the exported functions, the R6 class and how and when to use to use them.
  • As I mentioned above, the vignette is more useful if it is a fully compiled R Markdown with inputs and outputs. rgl can do this with WebGL. For instance, the following code will produce R markdown HTML output with manipulatable WebGL:
    ```{r setup, include=FALSE}
    library(rgl)
    library(Rpolyhedra)
    setupKnitr()
    ```

    ```{r tetra, webgl=TRUE}
    pol <- getPolyhedron(polyhedron.name = "tetrahedron")$getRGLModel()
    shade3d(pol, color = "red")
    ```

@kenarab
Copy link
Author

kenarab commented Jan 3, 2019

Hello @noamross:

Thank you for your time and your coaching and tips! We applied all of them. You can see them in this branch. Also we just uploaded a new release (v0.4.0) to CRAN.

We hope reviewers (originals or new ones) can take the time to assess Rpolyhedra.

Happy new year for all the Ropensci team.

Best, Ale.

@noamross
Copy link
Contributor

noamross commented Jan 6, 2019

@yulijia and @schloerke have agreed to review. Due date: 2019-01-30.

@kenarab
Copy link
Author

kenarab commented Jan 7, 2019

Thank you @noamross!

And than you @yulijia and @schloerke for accepting being our reviewers.

Best, Ale.

@yulijia
Copy link

yulijia commented Jan 30, 2019 •
edited
Loading

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 2.5 hours

Review Comments

I used Fedora release 28 and I was able to install the package from Github without any problem by devtools.

  • Function and variable naming is proper.
  • README.rmd is clear and easy to follow steps for installations.
  • devtools::check passes without any warnings or any errors.

This is quite a nice job, all demos are run successfully and very beautiful. The code is neat, but some of the package help pages make me confused. The following comments are addressing minor things that I didn't understand, it may need to be improved.

  1. The help page of Rpolyhedra-package and .onLoad() are same. What does .onLoad() used for?

  2. How to use scrapePolyhedra() and scrapePolyhedraSources()? Any examples in the help page? Or maybe they are internal functions?

@schloerke schloerke mentioned this issue Feb 1, 2019
Merged
@schloerke
Copy link

schloerke commented Feb 1, 2019 •
edited
Loading

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 3

Review Comments

README

  • Change to "scraped from publically available"
  • The install.packages line is not needed in instructions in development version. devtools::install_github will install them automatically.
  • Had to eventually use devtools::install_github("qbotics/Rpolyhedra", build_opts=NULL) as the vignette was not created using the default arguments. (Could also use the remotes package instead of devtools, as devtools is trying to not be responsible for install_github anymore.)
  • switchToFullDatabase failed to work on MacOS
> switchToFullDatabase()
Full Database needs to download data to home folder.  Agree [y/n]?:y
trying URL 'https://api.github.com/repos/qbotics/RpolyhedraDB/zipball/v0.4.0'
downloaded 5.1 MB

Error in file.copy(from = file.path(td, tmp.db.path, files.to.copy), to = getUserSpace(),  :
  more 'from' files than 'to' files

Documentation

  • The title for the exported function documentation to not be the function name (ex: getAvailablePolyhedra()). Typically, this short title is <= 5 words. (ex: Get available polyhedra)
  • Reformat the start of zzz.R to the code below.
    • By setting the "function" to "_PACKAGE", it will inherit from the DESCRIPTION file.
    • use \itemize for bulleted lists.
    • .onLoad should not be documented
#' @details
#' A polyhedra database scraped from:
#' \itemize{
#'    \item http://paulbourke.net/dataformats/phd/: PHD files as R6 objects and 'rgl'
#'          visualizing capabilities. The PHD format was created to describe the geometric
#'          polyhedra definitions derived mathematically <http://www.netlib.org/polyhedra/>
#'          by Andrew Hume and by the Kaleido program of Zvi Har'El.
#'    \item http://dmccooey.com/Polyhedra: Polyhedra text datafiles.
#' }
#'
#' @docType package
#' @importFrom R6 R6Class
#' @importFrom futile.logger flog.info
"_PACKAGE"

#' Executes code while loading the package.
#'
#' @param libname The library name
#' @param pkgname The package name
#' @noRd
.onLoad <- # (rest of code)

Examples

  • Not all exported functions contained an example. Missing for getPolyhedraObject and switchToFullDatabase. But both functions do not require examples IMO.

  • Do not library your package within the example for polyhedronToXML

  • Print the final output at the end of your examples. It's not much, but it lets the user know it worked.

    • ex: Add this final example line #'cubes to getAvailableSources
    • Do this for
      • getAvailablePolyhedra
      • getAvailableSources

Community Guidelines

  • Missing contribution guidelines in CONTRIBUTING or README.md

Automated Tests

  • Unfortunately, these do not pass on my machine. One failed.
    • Update: Passes now. Originally failed due to personal machine setup.
> test_check("Rpolyhedra")
trying URL 'https://api.github.com/repos/qbotics/RpolyhedraDB/zipball/v0.4.0'
downloaded 5.1 MB

── 1. Error: test on package lib functions (@test_package_lib.R#18)  ───────────
more 'from' files than 'to' files
1: with_mock(`Rpolyhedra::getUserSpace` = function() {
       .tmp.home.dir
   }, testthat::expect(with_mock(`Rpolyhedra::getDataEnv` = function() {
       "HOME"
   }, with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE"))), failure_message = "downloadRPolyhedraSupportingFiles error")) at testthat/test_package_lib.R:18
2: eval(code[[length(code)]], parent.frame())
3: eval(code[[length(code)]], parent.frame())
4: testthat::expect(with_mock(`Rpolyhedra::getDataEnv` = function() {
       "HOME"
   }, with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE"))), failure_message = "downloadRPolyhedraSupportingFiles error")
5: as.expectation.logical(ok, failure_message, info = info, srcref = srcref)
6: with_mock(`Rpolyhedra::getDataEnv` = function() {
       "HOME"
   }, with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE")))
7: eval(code[[length(code)]], parent.frame())
8: eval(code[[length(code)]], parent.frame())
9: with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE"))
10: eval(code[[length(code)]], parent.frame())
11: eval(code[[length(code)]], parent.frame())
12: downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE")
13: downloadRPolyhedraSupportingFiles()
14: file.copy(from = file.path(td, tmp.db.path, files.to.copy), to = getUserSpace(),
       recursive = TRUE)
15: stop("more 'from' files than 'to' files")

══ testthat results  ═══════════════════════════════════════════════════════════
OK: 651 SKIPPED: 0 FAILED: 1
1. Error: test on package lib functions (@test_package_lib.R#18)

Error: testthat unit tests failed
Execution halted

1 error ✖ | 0 warnings ✔ | 0 notes ✔

Packaging guidelines

  • Code style
    • Mix of camel case, dot case, and snake case
      • Most of the functions and variables are camel case. Might keep camel case for ease of use. (I prefer snake case.)
      • I understand the use of starting with a capital letter to signify that the variable is a R6 class and not a function.
      • Please do not use period separation.
      • Please be consistent with your case selection.
    • Use lintr's .lintr file with the contents of the code below and solve all remaining style lints found with lintr::lint_package()
linters: with_defaults(
    snake_case_linter, # do not allow snake case
    camel_case_linter = NULL, # allow camel case
    multiple_dots_linter, # do not allow dot sep
    commented_code_linter = NULL, # comment code is ok
    NULL
  )

Final thoughts

  • I've made a PR with most of the minor changes implemented.
  • The consistency of the code style is up to the authors / rOpenSci. It should be addressed or acknowledged that it's not going to change.

Remaining fixes needed for approval

Update Feb 4, 2019

  • My personal ~/.R folder was not writable. This should be writeable by default. This was my problem. By fixing the folder permissions, the test suite and switchToFullDatabase function works as expected.

@kenarab
Copy link
Author

kenarab commented Feb 2, 2019

Thank you for your feedback and advices @yulijia and @schloerke!
I just been father of tweens on last wednesday (1-30-2019) so I been full offline for some days. In next days we will analyse your feedback and write back.

We merged the PR @schloerke made, but wondering about which style code did you use. We are based on google style code (GSC). I don't know if RopenSci have a style code down to the level of GSC. Capital Letter starting convention was defined for R6 classes, as there was no reference to R6 classes in GSC.

It may be possible we have some unconsistency in the code, we will check. Maybe will change lintr config @schloerke PR us.

About broken tests and switchToFullDatabase is strange it is not working: we are using Travis CI and working on MacOS for developing and for every release we run all kinds of checks (A call to switchToFullDatabase is included in testcases) for it to work before publishing.
And then, for publishing the package on CRAN all test should work ok.

In which branch did you were working whe you had tests problems?

Thank you! We will reach you on next days.

@kenarab
Copy link
Author

kenarab commented Feb 2, 2019

about @yulijia question 2:

How to use scrapePolyhedra() and scrapePolyhedraSources()? Any examples in the help page? Or maybe they are internal functions?

They are development functions. We applied @noRd as @noamross suggested so there are no documentation.
For a package like this, intended to be reproducible, we would prefer have a call like @devRD (A vigentte for developers) for any dev or collaborator who wants to reproduce the backstage would be able to do it.
Reproducible features for the scraping process is not finished yet. We will research on this while working on this review.

@noamross
Copy link
Contributor

noamross commented Feb 4, 2019

@kenarab First, congratulations on becoming a new father, twice! 👶👶 Thank you @yulijia and @schloerke for your reviews.

Regarding code style: consistency is generally our rule. The pattern we find most frequent is people use one form for user-facing functions, a different form for internal functions and R6 or S4 types. With the split in this package user-facing and developer-facing functions, I mostly think that is important that there is consistency within each of these.

Per the reviewer's questions above, the CONTRIBUTING.md file would be an excellent place to explain the overall scraping/developer workflow, referring the reader to the documentation in the source code for details on specific functions.

@sckott is our expert on mocking test functions, he might have some insight into the test failure above.

@schloerke
Copy link

Updating here as well. My personal configuration had ~/.R require sudo to make new folders. This is non standard. By fixing my folder permissions, it fixed the function issue and test suite failure. Sorry for the odd scare!

@kenarab
Copy link
Author

kenarab commented Feb 4, 2019 •
edited
Loading

Thank you @noamross! The twins are in neonatology, but they are very good. They are growing really fast and will go to home soon.

Dear @yulijia and @schloerke and @noamross . We implemented most of suggestions.

  • We configured lintr for CamelCase and DotCase. Removed Snake case and found some internal inconsistencies, but most were related to dependencies, specifically testthat. For avoiding lintr messages added testthat spacename in that callings. But it is specific to some lintr version and others no.
  • About code style we still using google code style. With exception of R6 objects, as discussed previously. This was included in CONTRIBUTING.md as matters to developers only
  • We included a CONTRIBUTING.md which explain some basics of the package for developers. We will improve the description in next versions. For example, for people wanting to program scrapers for other sources.
  • About @yulijia question The help page of Rpolyhedra-package and .onLoad() are same. What does .onLoad() used for?: @schloerke the other reviewer made a PR fixing this documentation issue. .onLoad function is a standard function for R package with initialization scripts regarding a package.

Thank you very much for your time.

Best, Ale

PS. We made a dev-ropensci branch for developing suggestions. We are merging in master when they are ready. You can check master branch, but for I suggest PR should be made over dev-ropensci branch instead of master

@yulijia
Copy link

yulijia commented Feb 14, 2019

Hi @kenarab, sorry for the delayed reply.
Thank you for answer all my questions, I have checked all the document again, and they are fine now.
Congratulation for the twin.

@noamross
Copy link
Contributor

Thank you all. @yulijia and @schloerke, please let us know if @kenarab's changes have addressed all your comments. If you have any additional comments on interface or use cases that you may not have gotten to while working through testing and documentation issues, feel free to provide these, as well.

@schloerke
Copy link

Good to me!

Took me a couple times to realize the Google Coding Style is a mixed style to help identify a variable vs a package function. I've been trained on a "all or nothing" style (preferring snake case, which google does not like).

@kenarab
Copy link
Author

kenarab commented Feb 15, 2019 via email

@noamross
Copy link
Contributor

Approved! 🎉Thanks @kenarab for submitting and going through what has been a long read, and and @yulijia and @schloerke for your reviews!

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
  • Add the rOpenSci footer to the bottom of your README
    " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
  • Fix any links in badges for CI and coverage to point to the ropensci URL.
  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing.

We recently published a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

@kenarab
Copy link
Author

kenarab commented Feb 25, 2019

Wow! Gr8!! Thank you again @noamross, @schloerke @yulijia for your time.

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.

  • Add the rOpenSci footer to the bottom of your README
    " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"

  • Fix any links in badges for CI and coverage to point to the ropensci URL.

  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

  • Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

I added the 3 of you as reviewers. If you want changes, please make a PR with them.

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing.

We recently published a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

Excellent! We will read the corresponding chapters and planning to write a blog post for being included in (https://ropensci.org/tech-notes/) and/or (https://ropensci.org/tech-notes/)(https://ropensci.org/blog/)

As I already transferred the repo I need admin privileges for inviting @leobelen to rOpenSci and giving him admin access too for pushing the changes requesting in onboarding process.

Best, Ale.

@noamross
Copy link
Contributor

Great. Admin privileges assigned. Please don't include editors as rev contributors, we would be on too many 🙂 .

@kenarab
Copy link
Author

kenarab commented Feb 25, 2019 via email

@stefaniebutland
Copy link
Member

@kenarab I'm rOpenSci's Community Manager, here to say we would love to feature a post about Rpolyhedra on our blog.

This link will give you many examples of blog posts by authors of peer-reviewed packages so you can get an idea of the style and length you prefer: https://ropensci.org/tags/software-peer-review/. Here are some technical and editorial guidelines for contributing a blog post: https://github.com/ropensci/roweb2#contributing-a-blog-post.

Please let me know when you might want to submit a draft. Happy to answer any questions.

@noamross noamross closed this as completed May 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@noamross noamross

Labels
6/approved package topic:mathematics
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@schloerke @noamross @sckott @yulijia @kenarab @stefaniebutland