search

ropensci / software-review

rOpenSci Software Peer Review.
289 stars 104 forks source link

Submission: c14bazAAR #333

Closed nevrome closed 4 years ago

nevrome commented 5 years ago

Submitting Author: Clemens Schmid (@nevrome)
Repository: https://github.com/ISAAKiel/c14bazAAR Version submitted: 1.0.3.9000 Editor: @melvidoni Reviewer 1: @benmarwick
Reviewer 2: @ercrema Archive: TBD
Version accepted: TBD 

Package: c14bazAAR
Title: Download and Prepare C14 Dates from Different Source Databases
Description: Query different C14 date databases and apply basic data cleaning, merging and calibration steps.
Version: 1.0.3.9000
Authors@R: 
    c(person(given = "Clemens",
             family = "Schmid",
             role = c("aut", "cre", "cph"),
             email = "clemens@nevrome.de",
             comment = c(ORCID = "0000-0003-3448-5715")),
      person(given = "Dirk",
             family = "Seidensticker",
             role = "aut",
             email = "dirk.seidensticker@gmail.com",
             comment = c(ORCID = "0000-0002-8155-7702")),
      person(given = "Daniel",
             family = "Knitter",
             role = "aut",
             email = "knitter@geographie.uni-kiel.de",
             comment = c(ORCID = "0000-0003-3014-4497")),
      person(given = "Martin",
             family = "Hinz",
             role = "aut",
             email = "martin.hinz@iaw.unibe.ch",
             comment = c(ORCID = "0000-0002-9904-6548")),
      person(given = "David",
             family = "Matzig",
             role = "aut",
             email = "matzigdavid@gmail.com",
             comment = c(ORCID = "0000-0001-7349-5401")),
      person(given = "Wolfgang",
             family = "Hamer",
             role = "aut",
             email = "hamer@geographie.uni-kiel.de",
             comment = c(ORCID = "0000-0002-5943-5020")),
      person(given = "Kay",
             family = "Schmütz",
             role = "aut",
             email = "kschmuetz@ufg.uni-kiel.de"),
      person(given = "Nils",
             family = "Mueller-Scheessel",
             role = "ctb",
             email = "nils.mueller-scheessel@ufg.uni-kiel.de",
             comment = c(ORCID = "0000-0001-7992-8722")))
URL: https://github.com/ISAAKiel/c14bazAAR
BugReports: https://github.com/ISAAKiel/c14bazAAR/issues
Depends: R (>= 3.4.0)
Language: en_GB
License: GPL-2 | file LICENSE
Encoding: UTF-8
LazyData: true
Imports:
    crayon (>= 1.3.4),
    data.table (>= 1.11.4),
    dplyr (>= 0.7.2),
    httr (>= 1.4.0),
    magrittr (>= 1.5),
    pbapply (>= 1.3-3),
    rlang (>= 0.1.1),
    stats (>= 3.4.0),
    tibble (>= 1.3.3),
    tidyr (>= 0.6.3)
Suggests:
    Bchron,
    countrycode,
    dataverse,
    ggplot2,
    ggridges,
    knitr,
    lwgeom,
    mapview,
    openxlsx,
    plyr,
    rgeos,
    rmarkdown,
    rnaturalearth,
    rworldmap,
    rworldxtra,
    sf,
    stringdist,
    testthat
RoxygenNote: 6.1.1
VignetteBuilder: knitr

Scope

c14bazAAR was created to access radiocarbon dates from openly accessible archives. All functions are related to data download and data preparation.

Mostly archaeologists as most radiocarbon databases currently accessible via c14bazAAR contain radiocarbon dates from archaeological contexts. The package could also be of interest for geographers and other kinds of ecologists -- especially if non-archaeological data sources are added to the portfolio.

No. Not that I'm aware of.

Technical checks

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

Publication options

It already is on CRAN in version 1.0.3.

JOSS Options - [x] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [x] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [x] The package is deposited in a long-term repository with the DOI: 10.17605/OSF.IO/3DS6A - (*Do not submit your package separately to JOSS*)

Code of conduct

melvidoni commented 5 years ago

Hello @nevrome and thank you for your submission. I have noticed that you haven't ticked the rOpenSci's CoC box. Is that for a particular reason? It is fundamental for us to have that, otherwise we cannot continue the process.

nevrome commented 5 years ago

@melvidoni No idea how I managed to forget that. Done now.

melvidoni commented 5 years ago

Editor checks:

Editor comments

Thank you for submitting, I'll be your handling editor. I run goodpractices but apparently you are clear to go, so I will start finding reviewers for the package. I'll let you know once they are in, and we have a review deadline.

Reviewers: @benmarwick and @ercrema Due date: Monday 16th of September

melvidoni commented 5 years ago

I am in the search for a second reviewer. I will update here and give a review deadline as soon as someone accepts my invitation. Apologies for the delay, @nevrome

melvidoni commented 4 years ago

Reviewers have been assigned, @nevrome. Thanks to @benmarwick and @ercrema. The review deadline is Monday 16th of September.

benmarwick commented 4 years ago

Package Review

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [x] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 2

Review Comments

This is an exemplary package that was a delight to review. It is a perfect fit for rOpenSci and deserves a wide audidence.

I ran some of the code in the README.md and everything worked as expected. I appreciated the comments that some code would take long to run! The vignette is beautifully written and elegantly and efficiently demonstrates some typical uses of the package. The function help files are thorough and encyclopedic, and rich with examples. All of the metadata files are excellent and have been prepared to the highest standards. I inspected a sample of functions and everything I saw looked excellent.

One suggestion for improvement: in get_country_thesaurus and similar get_ functions I see we are downloading data, eg. from "https://raw.githubusercontent.com/ISAAKiel/c14bazAAR/master/data-raw/material_thesaurus.csv". Looking in data-raw/ I see that these files are not large (none more than 100 KB). So I suggest that the authors make these exported data objects in the package so users don't have to download again.

I used pkgreviewr to help prepare this review, the full contents of my review are in the fold below

pkgreviewr output

--- output: html_notebook: toc: true toc_float: true editor_options: chunk_output_type: console --- ```{r setup, include = FALSE} knitr::opts_chunk$set( collapse = TRUE, comment = "#>", fig.path = "man/figures/README-", out.width = "100%" ) library(magrittr) library(devtools) ``` # `c14bazAAR` - package review ## **Reviewer:** [\@benmarwick](https://github.com/benmarwick) ### Review Submitted: **`r cat(sprintf("**Last updated:** %s", Sys.Date()))`** ***
This report contains documents associated with the review of **rOpenSci** submitted package: ### **`c14bazAAR`: ropensci/software-review** issue [\#NA](https://github.com/ropensci/onboarding/issues/).
## Package info **Description:** Query different C14 date databases and apply basic data cleaning, merging and calibration steps. **Author:** `r c(person(given = "Clemens", family = "Schmid", role = c("aut", "cre", "cph"), email = "clemens@nevrome.de", comment = c(ORCID = "0000-0003-3448-5715")), person(given = "Dirk", family = "Seidensticker", role = "aut", email = "dirk.seidensticker@gmail.com", comment = c(ORCID = "0000-0002-8155-7702")), person(given = "Daniel", family = "Knitter", role = "aut", email = "knitter@geographie.uni-kiel.de", comment = c(ORCID = "0000-0003-3014-4497")), person(given = "Martin", family = "Hinz", role = "aut", email = "martin.hinz@iaw.unibe.ch", comment = c(ORCID = "0000-0002-9904-6548")), person(given = "David", family = "Matzig", role = "aut", email = "matzigdavid@gmail.com", comment = c(ORCID = "0000-0001-7349-5401")), person(given = "Wolfgang", family = "Hamer", role = "aut", email = "hamer@geographie.uni-kiel.de", comment = c(ORCID = "0000-0002-5943-5020")), person(given = "Kay", family = "Schmütz", role = "aut", email = "kschmuetz@ufg.uni-kiel.de"), person(given = "Nils", family = "Mueller-Scheessel", role = "ctb", email = "nils.mueller-scheessel@ufg.uni-kiel.de", comment = c(ORCID = "0000-0001-7992-8722")))` **repo url:** **website url:** <> ## Review info #### See [reviewer guidelines](https://ropensci.github.io/dev_guide/reviewerguide.html) for further information on the rOpenSci review process. **key review checks:** - Does the code comply with **general principles in the [Mozilla reviewing guide](https://mozillascience.github.io/codeReview/review.html)**? - Does the package **comply with the [ROpenSci packaging guide](https://ropensci.github.io/dev_guide/building.html)**? - Are there **improvements** that could be made to the **code style?** - Is there **code duplication** in the package that should be reduced? - Are there **user interface improvements** that could be made? - Are there **performance improvements** that could be made? - Is the [**documentation**](https://ropensci.github.io/dev_guide/building.html#documentation) (installation instructions/vignettes/examples/demos) **clear and sufficient**? Please be respectful and kind to the authors in your reviews. The rOpenSci [code of conduct](https://ropensci.github.io/dev_guide/policies.html#code-of-conduct) is mandatory for everyone involved in our review process. *** ### session info ```{r sessionInfo} sessionInfo() ``` ```{r pkg_dir, echo = F} pkg_dir <- "/Users/bmarwick/Desktop/c14bazAAR-review/c14bazAAR" ``` ## Test installation ### test local `c14bazAAR` install: ```{r test-local} install(pkg_dir, dependencies = T, build_vignettes = T) ``` ```{r github-rm} remove.packages("c14bazAAR") ``` #### **comments:** No issues with local install of `c14bazAAR`. *** ### test install of `c14bazAAR` from GitHub with: ```{r test-github} devtools::install_github("ISAAKiel/c14bazAAR", dependencies = T, build_vignettes = T) ``` #### **comments:** No issues with GitHub install of `c14bazAAR` *** ## Check package integrity ### run checks on `c14bazAAR` source: ```{r check-checks} devtools::check(pkg_dir) ``` #### **comments:** No issues with checks of `c14bazAAR`: ``` ── R CMD check results ─────────────────── c14bazAAR 1.0.3.9000 ──── Duration: 32.6s 0 errors ✔ | 0 warnings ✔ | 0 notes ✔ ``` *** ### run tests on `c14bazAAR` source: ```{r check-tests} devtools::test(pkg_dir) ``` #### **comments:** No issues with tests of `c14bazAAR`: ``` ══ Results ════════════════════ Duration: 48.8 s OK: 68 Failed: 0 Warnings: 0 Skipped: 0 ``` *** ### check `c14bazAAR` for goodpractice: ```{r test-goodpractice} goodpractice::gp(pkg_dir) ``` #### **comments:** Many warnings of 'write unit tests for all functions, and all package code in general. 42% of code lines are covered by test cases.' Looks like these warning are appearing on unexported helper functions that do checking and formatting. I don't believe it is a high priority to have test coverage on these. *** ## Check package metadata files ### inspect - #### [README](https://github.com/ISAAKiel/c14bazAAR) - #### [DESCRIPTION](https://github.com/ISAAKiel/c14bazAAR/blob/master/DESCRIPTION) - #### [NAMESPACE](https://github.com/ISAAKiel/c14bazAAR/blob/master/NAMESPACE) ### spell check ```{r spell-check} devtools::spell_check(pkg_dir) ``` #### **comments:** All of these metadata files are excellent and have been prepared to the highest standards. *** ## Check documentation online documentation: **<>** * Is the [documentation](https://ropensci.github.io/dev_guide/building.html#documentation) (installation instructions/vignettes/examples/demos) clear and sufficient? ### test `c14bazAAR` function help files: ```{r test-help} help(package = "c14bazAAR") ``` #### **comments:** The function help files are thorough and encyclopeadic, and rich with examples. *** ### test `c14bazAAR` vignettes: ```{r test-vignettes} vignette(package = "c14bazAAR") ``` #### **comments:** The vignette is beautifully written and elegantly and efficiently demonstrates some typical uses of the package. *** ## Test functionality: - Are there **user interface improvements** that could be made? - Are there **performance improvements** that could be made? ```{r free-style} library("c14bazAAR") ``` ```{r parse-functions} exports <-ls("package:c14bazAAR") exports ``` #### **comments:** I ran some of the code in the README.md and everything worked as expected. I appreciated the comments that some code would take long to run! *** ## Inspect code: - Does the package **comply with the [ROpenSci packaging guide](https://ropensci.github.io/dev_guide/building.html)**? * good [function & variable naming?](https://ropensci.github.io/dev_guide/building.html#function-and-argument-naming) * good [dependency management](https://ropensci.github.io/dev_guide/building.html#package-dependencies)? - Are there **improvements** that could be made to the [**code style?**](https://ropensci.github.io/dev_guide/building.html#code-style) - Is there **code duplication** in the package that should be reduced? ```{r inspect-code, eval=FALSE} pkgreviewr::pkgreview_print_source("c14bazAAR") ```

#### **comments:** I inspected a sample of functions and everything I saw looked excellent. One suggestion for improvement: in `get_country_thesaurus` and similar `get_` functions I see we are downloading data, eg. from `"https://raw.githubusercontent.com/ISAAKiel/c14bazAAR/master/data-raw/material_thesaurus.csv"`. Looking in `data-raw/` I see that these files are not large (none more than 100 KB). So I suggest that the authors make these exported data objects in the package so users don't have to download again. ## Review test suite: See guidance on [testing](https://ropensci.github.io/dev_guide/building.html#testing) for further details. ### test coverage ```{r pkg_coverage} covr::package_coverage(pkg_dir) ``` ### inspect [tests](https://github.com/ISAAKiel/c14bazAAR/blob/master/tests/testthat) ``` c14bazAAR Coverage: 42.53% R/get_14sea.R: 0.00% R/get_adrac.R: 0.00% R/get_austarch.R: 0.00% R/get_calpal.R: 0.00% R/get_context.R: 0.00% R/get_eastafrica.R: 0.00% R/get_eubar.R: 0.00% R/get_euroevol.R: 0.00% R/get_palmisano.R: 0.00% R/get_radon.R: 0.00% R/get_radonb.R: 0.00% R/helpers_db_urls.R: 0.00% R/zzz.R: 0.00% R/get_all_dates.R: 40.00% R/c14_date_list_basic.R: 42.11% R/helpers_general.R: 62.50% R/c14_date_list_spatial_determine_country_by_coordinate.R: 71.83% R/c14_date_list_fuse.R: 81.82% R/c14_date_list_spatial_finalize_country_name.R: 87.50% R/c14_date_list_classify_material.R: 89.47% R/c14_date_list_spatial_standardize_country_name.R: 92.50% R/c14_date_list_convert.R: 92.86% R/c14_date_list_duplicates_remove.R: 93.02% R/c14_date_list_spatial_coordinate_precision.R: 93.10% R/c14_date_list_duplicates_mark.R: 94.87% R/c14_date_list_calibrate.R: 95.92% R/c14_date_list_enforce_types.R: 96.43% R/c14_date_list_order_variables.R: 97.22% R/helpers_thesauri.R: 100.00% ``` #### **comments:** I'm satisfied with the testing coverage. ***

ercrema commented 4 years ago

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

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [x] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 1

Review Comments

This is an excellent package that will undoubtedly help many users handling radiocarbon datasets. I tested several commands and had no problems at all. I strongly recommend its publication on rOpenSci. Some minor points below:

melvidoni commented 4 years ago

Reviews are in @nevrome, so we will await for your reply/changes/actions.

nevrome commented 4 years ago

Thank you for your time and valuable feedback, @benmarwick and @ercrema! And of course thank you for the organisation so far, @melvidoni. I spoke with @dirkseidensticker and we decided to answer your requests in the following way:

@benmarwick

  1. This unusual raw data download setup was established on purpose to be able to react fast in case of changes in the location (url) or content (new country names, new materials) of the source databases. A CRAN release can take a relatively long time. With this setup we are a lot more flexible and can make things work again immediately.

@ercrema

  1. https://github.com/ISAAKiel/c14bazAAR/commit/b21770f435279cc13c08b9204fd4698dde33619b establishes a separation of a Background and a Code Summary section. This structure is also used by other JOSS papers. I hope that's what you had in mind?
  2. DOI added with https://github.com/ISAAKiel/c14bazAAR/commit/ab42faa1c495a19f5f9372ae2cccf19deada76b5.
  3. I guess the relevant PR is this one here: https://github.com/andrewcparnell/Bchron/pull/12, ja? Interesting! I'll keep an eye on it.
  4. https://github.com/ISAAKiel/c14bazAAR/commit/b4889e5df9bfb702c969ce12ea7f852767423671 adds some example code for these functions.
  5. Both Dirk and I like the idea of simplifying the interface in this way. I wrote a first draft here and we will explore it further.

Beyond these changes I also added you to the DESCRIPTION file: https://github.com/ISAAKiel/c14bazAAR/commit/2d2ffc4f0a4af8c8dc8344f1c31b2c014dddb50b. Is your information correct?

ercrema commented 4 years ago
  1. Yes precisely.
  2. Great!
  3. Yes indeed - Of course rcarbon and Oxcal might have got this wrong, but I think the calibration curve is reporting errors in sigma and not sigma^2
  4. Great - and they all work fine.
  5. Looks great!
benmarwick commented 4 years ago

Thanks, that makes sense, sounds good!

melvidoni commented 4 years ago

Please, @ercrema and @benmarwick , let me know when you think the package is ready!

benmarwick commented 4 years ago

I think it's ready!

ercrema commented 4 years ago

Agreed!

melvidoni commented 4 years ago

Approved! Thanks @nevrome for submitting and @ercrema and @benmarwick for your reviews!

To-dos:

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 love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've started putting together 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.

nevrome commented 4 years ago

Thanks for these instructions, @melvidoni.

I was not fully aware that the package is intended to be hosted at the "ropensci" github organization at the end of the review. I would rather not do that. c14bazAAR is currently placed at the ISAAKiel github organization and I would prefer not to move it. ISAAK is a brand with recognition value in the field of computational archaeology.

What are the consequences of this decision and would you advise me to reconsider?

benmarwick commented 4 years ago

If I could suggest one possible option for the best of both worlds: use multiple remotes. Would you consider to mirror the pkg on rOpenSci, and keep the official repo on ISAAK? Clemens can then simultaneously push to both remotes to easily keep them in sync.

melvidoni commented 4 years ago

Hello @nevrome . We are discussing this with the editors. Please bear with me while we try to reach a consensus on what to do.

stefaniebutland commented 4 years ago

Hi @nevrome. I'm rOpenSci's Community Manager. Once the archiving issue is resolved, would you be interested in writing a blog post about some aspect of c14bazAAR? To the best of my knowledge, this is the first Archaeology-focused package to go through rOpenSci software peer review so a post would garner some attention. @benmarwick said he'd be happy to contribute if you feel that's appropriate.

This tag gets you (diverse) examples of blog posts by authors of peer-reviewed packages: https://ropensci.org/tags/software-peer-review/.

Here are some technical and editorial guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post. Publication date is flexible. I like to get a draft via pull request a week before the planned publication date so I can review.

Happy to answer any questions.

nevrome commented 4 years ago

@stefaniebutland Ja - absolutely! That's a great idea and I would love to do that.

I'm sure you have some ideas, which aspects of c14bazAAR might be the most interesting for this purpose @benmarwick. I have some as well. Let's discuss this, when the archiving is sorted out.

melvidoni commented 4 years ago

Hello @nevrome, apologies for the extended delay. The package needs to be under our GitHub repo, but there's no reason it can't also be under another GitHub repo, too, and there are several reasonable solutions for this (multiple remotes, update by CI, etc.). Could you set up something like that?

nevrome commented 4 years ago

@melvidoni The delay was mutual: I just started a new job and was not able to think much about this project. I will do some research about the possible solutions and come back to you asap.

nevrome commented 4 years ago

@melvidoni I discussed this now with the ISAAK people and decided to transfer the package to the ropensci organisation. I will establish a mirror system via CI to keep a copy of the repo at the ISAAKiel organisation.

Unfortunately I was not able to start the transfer: You don’t have the permission to create public repositories on ropensci. What's the problem here?

melvidoni commented 4 years ago

Hello @nevrome. You need to first transfer the repo (you need to be part of the rOpenSci org, and transfer not create the repo). I have sent you an invitation for a team at rOpenSci; I resent it just now, so please check it. After that, try to transfer the repo again.

After transfer is done, ping me and I'll assign you admin rights on it. That is how we always do it.

nevrome commented 4 years ago

Ah - just got the invitation. Thank you

stefaniebutland commented 4 years ago

@nevrome Great to hear things are sorting out. I'm keen to host a blog post when you have time to work on this. Both you and @benmarwick are in rOpenSci Slack so we could move the blog post discussion to a DM there.

nevrome commented 4 years ago

@melvidoni: I fulfilled all the ToDos in https://github.com/ropensci/software-review/issues/333#issuecomment-531599658 now. Except for the blog post.

arfon commented 4 years ago
  • [ ] Submit to JOSS using the Zenodo DOI. They will tag it for expedited review.

:wave: @melvidoni - this isn't quite correct sorry, we actually need authors to submit to JOSS using the repository address (https://github.com/ropensci/c14bazAAR). Is there an rOpenSci template I can propose a modification to?

melvidoni commented 4 years ago
  • [ ] Submit to JOSS using the Zenodo DOI. They will tag it for expedited review.

👋 @melvidoni - this isn't quite correct sorry, we actually need authors to submit to JOSS using the repository address (https://github.com/ropensci/c14bazAAR). Is there a rOpenSci template I can propose a modification to?

Yes, there is. Let me mention this to the other editors first, please.