NIEHS / chopin

Computation of Spatial Data by Hierarchical and Objective Partitioning of Inputs for Parallel Processing
https://niehs.github.io/chopin/
Other
10 stars 3 forks source link

error running README examples #50

Open cole-brokamp opened 8 months ago

cole-brokamp commented 8 months ago

srtm <- terra::unwrap(readRDS("../../tests/testdata/nc_srtm15_otm.rds")) fails for me because I think the code is intended to work only if run from the development version of the package.

We could use a system.file() call like you did with the example sf data to get that to work (although tests are not installed by default if using the “testthat” package for tests; you might have to include this example in the package itself)

Using a README.Rmd to knit a README file would be a good way to include these in the CI process.

sigmafelix commented 8 months ago

Thank you for reporting the error. README.md is now based on README.Rmd to make sure that the code inside runs without errors. nc.shp from sf package is used in the example, but other data were reused from files in tests/testdata to reduce dependencies of the package. I rewrote the code to make the files be downloaded from the raw link of the file in chopin repository. Would it matter in submission to ROpenSci or CRAN? If it is the case, I will find another way to get a permanent link (e.g., in Zenodo or figshare) for example datasets.

Thank you for your help!

cole-brokamp commented 8 months ago

You could put the example data in the /inst folder and then it would be installed with the package.

For example, the sf example data you are calling is located here: https://github.com/r-spatial/sf/tree/main/inst/shape and loaded with the command you have in the example: ncpoly <- system.file("shape/nc.shp", package = "sf")

Taking a similar approach for example data would prevent problems with downloading a file from the internet being required to run any examples.

I'd also suggest to remove the code about installing required packages for the readme example. If the README requires packages that are not installed with the package as dependencies, you can include them as suggested dependencies that will get installed. Or you could use rlang::is_installed() to check and tell user to install if not available. Cutting out a lot of this boilerplate code (especially if we are assuming users have a basic understanding of R) will likely increase the clarity of the README.

sigmafelix commented 8 months ago

The example data were not included in the package intentionally to reduce the file size of the built package (./tests/testdata folder is ~25 MB). Thank you for letting me know rlang::is_installed(). I found that rlang::check_installed() installs packages if there are any missing packages in users' system and loads them. Installation part in README.md gets dependencies = TRUE and its explanation for installing all required and suggested packages when chopin is installed.

I agree on making README succinct by removing boilerplate codes. Do you think the latest push includes boilerplate codes? Thank you!

cole-brokamp commented 8 months ago

Just my two cents, but dependencies = TRUE shouldn't be required because dependencies for an R package are automatically installed before the package is installed. To simplify even further, you could only suggest one way to install the package instead of three different ways.

You could host the data somewhere else if you don't want to include it in the package, but right now the README wouldn't be run-able without someone cloning the package to their computer. A workaround might be to host the data with the github release of the package as an asset.

sigmafelix commented 8 months ago

To make the README.md runnable, I will include the datasets used in README in inst/extdata as initially suggested. I think 25 MB of data might not be a big problem as the package size will still abide by 100 MB rules in CRAN. For installation scripts, I will keep remotes::install_github. Thank you for the comments!