openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[PRE REVIEW]: localcovid19now: processing and mapping COVID-19 case data at subnational scales #4791

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@sjbeckett<!--end-author-handle-- (Stephen Beckett) Repository: https://github.com/sjbeckett/localcovid19now Branch with paper.md (empty if default branch): Version: 0.0.0.9900 Editor: !--editor-->@galessiorob<!--end-editor-- Reviewers: !--reviewers-list-->@richelbilderbeek<!--end-reviewers-list-- Managing EiC: Arfon Smith

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/a350d5e6dd0610b411aee8fdcc05472d"><img src="https://joss.theoj.org/papers/a350d5e6dd0610b411aee8fdcc05472d/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/a350d5e6dd0610b411aee8fdcc05472d/status.svg)](https://joss.theoj.org/papers/a350d5e6dd0610b411aee8fdcc05472d)

Author instructions

Thanks for submitting your paper to JOSS @sjbeckett. Currently, there isn't a JOSS editor assigned to your paper.

@sjbeckett if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @editorialbot is here to help you find and assign reviewers and start the main review. To find out what @editorialbot can do for you type:

@editorialbot commands
editorialbot commented 2 years ago

Hello human, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.78 s (411.2 files/s, 49242.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                           174           4217            534          13957
JSON                             2              0              0          10207
R                              125           1004           2196           3725
XML                              1              0              0            525
CSS                              3             98             52            442
JavaScript                       3             64             32            256
Markdown                         3            144              0            237
TeX                              1             40              0            226
YAML                             5             29             11            147
SVG                              1              0              1             11
Rmd                              1             16             26              5
-------------------------------------------------------------------------------
SUM:                           319           5612           2852          29738
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 968

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

editorialbot commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1073/pnas.2100970118 is OK
- 10.1038/s41597-022-01245-1 is OK
- 10.21105/joss.02376 is OK
- 10.18637/jss.v084.i06 is OK

MISSING DOIs

- 10.1098/rsif.2010.0142 may be a valid DOI for title: Modelling the influence of human behaviour on the spread of infectious diseases: a review
- 10.1101/2020.05.03.20089524 may be a valid DOI for title: Awareness-driven behavior changes can shift the shape of epidemics away from peaks and toward plateaus, shoulders, and oscillations
- 10.1101/2020.08.24.20181271 may be a valid DOI for title: Real-time, interactive website for US-county-level COVID-19 event risk assessment
- 10.21105/joss.03290 may be a valid DOI for title: covidregionaldata: Subnational data for COVID-19 epidemiology
- 10.31234/osf.io/6m5p4 may be a valid DOI for title: Imagining a personalized scenario selectively increases perceived risk of viral transmission for older adults
- 10.31234/osf.io/v8tdf may be a valid DOI for title: Real-time Interventions Counteract COVID-19 Risk Misestimation in the United States
- 10.1016/s1473-3099(20)30120-1 may be a valid DOI for title: An interactive web-based dashboard to track COVID-19 in real time
- 10.1590/scielopreprints.362 may be a valid DOI for title: Monitoring the number of COVID-19 cases and deaths in Brazil at municipal and federative units level
- 10.1503/cmaj.75262 may be a valid DOI for title: Open access epidemiologic data and an interactive dashboard to monitor the COVID-19 outbreak in Canada
- 10.1016/s2468-2667(20)30225-5 may be a valid DOI for title: COVID-19 in New Zealand and the impact of the national response: a descriptive epidemiological study

INVALID DOIs

- None
sjbeckett commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

arfon commented 2 years ago

@sjbeckett - thanks for your submission to JOSS. We're currently managing a large backlog of submissions and the editor most appropriate for your area is already rather busy.

For now, we will need to waitlist this paper and process it as the queue reduces. Thanks for your patience!

kthyng commented 2 years ago

Hi @galessiorob! Could you edit this submission?

kthyng commented 2 years ago

@editorialbot invite @galessiorob as editor

editorialbot commented 2 years ago

Invitation to edit this submission sent!

galessiorob commented 2 years ago

@editorialbot add @galessiorob as editor

editorialbot commented 2 years ago

Assigned! @galessiorob is now the editor

galessiorob commented 2 years ago

Hi @sjbeckett

I'm looking forward to working with you on this review. I'll send out review invites shortly, and once we have two reviewers we can open a fresh issue (the actual review issue.) If you have any suggestions for reviewers, please let me know (post a comment with their handles), and I can reach out to them.

galessiorob commented 2 years ago

@editorialbot add @richelbilderbeek as reviewer

editorialbot commented 2 years ago

@richelbilderbeek added to the reviewers list!

editorialbot commented 2 years ago

Hello @richelbilderbeek, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set joss-paper as branch

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers
richelbilderbeek commented 2 years ago

Here is my review! See https://github.com/richelbilderbeek/review_localcovid19now to see it in plaintext :-)

Abstract

First and foremost, I think the authors did an excellent job in collecting all information about COVID prevalance! I think the package is useful and relevant.

However, when only running the examples both README.md and the code examples, there are plenty of needless warnings. Similar to having spelling errors in an academic manuscript, this comes accross as needlessly sloppy. Also, not using a uniform coding standard is another such thing to make the package come accross as needlessly sloppy (for example, calc_risk, addNewGeoms and LoadAlgeria each follow a different naming convention). Additionally, most (all?) examples are put into dontrun tags, which is fine for fuction that take a long time, but this happens for short functions as well. This gives the incorrect impression that the authors do not want to have examples that are actually ran, and hence, checked (the calc_risk function is a good example)! I encourage the authors to fix these things, so that localcovid19now makes an even better impression, as I feel they did important work that should not be underappreciated.

Review checklist

Conflict of interest

Code of Conduct

General checks

Yes

Yes, an MIT license

Yes, the submitting author made major contributions to the software.

Unsure if the full list of paper authors is appropriate and complete: Below I made a table from the author list and the GitHub Contributor list at https://github.com/sjbeckett/localcovid19now/graphs/contributors. From that I conclude:

Name Author Contributor
Beckett Yes Yes
Brandel-Tanis Yes Yes
Chande Yes Yes
Johnson No [1] Yes
Guyen Yes Yes
Rishishwar Yes No
Andris Yes No
Weitz Yes No

Functionality

Yes

Loading the Phillipines dataset on its own, after authentication, fails:

> googledrive::drive_auth(email = TRUE)
> Philippines <- LoadPhilippines()
Error in `gargle::response_process()`:
! Client error: (403) Forbidden
Insufficient Permission: Request had insufficient authentication scopes.
• domain: global
• reason: insufficientPermissions
• message: Insufficient Permission: Request had insufficient authentication scopes.
Run `rlang::last_error()` to see where the error occurred.

However, the Phillipes can be loaded by using r LoadCountries:

> GLOBALMAP <- LoadCountries()

 LoadPhilippines 

 LoadAlgeria 

 LoadAustralia 

NA

Documentation

The authors clearly state what the package does.

The authors do not state the intended audience.

No and this is not needed: the installation installs all dependencies.

When running the code in README.md, one gets as needless warning:

> Malaysia <- LoadMalaysia()
Rows: 16096 Columns: 25                                                                 
── Column specification ──────────────────────────────────────────────────────────────────
Delimiter: ","
chr   (1): state
dbl  (23): cases_new, cases_import, cases_recovered, cases_active, cases_cluster, case...
date  (1): date

ℹ Use `spec()` to retrieve the full column specification for this data.
ℹ Specify the column types or set `show_col_types = FALSE` to quiet this message.

If r googledrive::drive_auth(email = TRUE) is not run, then, indeed, r Philippines <- LoadPhilippines() fails. However, also r GLOBALMAP <- LoadCountries() fails if r googledrive::drive_auth(email = TRUE) is not run. I would enjoy to see this documented as such:

if (has_credentials()) {
  Philippines <- LoadPhilippines()
  GLOBALMAP <- LoadCountries()
} else {
  message(
    "You need to have Google Drive credientials ",
    "to use 'Philippines' and 'GLOBALMAP'. ",
    "To authenticate, use:",
    "",
    "'googledrive::drive_auth(email = TRUE)'"
  )
}

Running PerCapitaMap_leaflet on US gives a needless warning:

> PerCapitaMap_leaflet(US,100000)
Warning message:
In pal(percapcases) :
  Some values were outside the color scale and will be treated as NA

Running PerCapitaMap_tmap on US gives a needless warning:

> PerCapitaMap_tmap(US,100000)
Variable(s) "percapcases" contains positive and negative values, so midpoint is set to 0. Set midpoint = NA to show the full spectrum of the color palette.
Warning message:
Values have found that are less than the lowest break 

Running this line fails:

MAP = EventMap_tmap(US,100,US$AB,projection="ESPG:5070")
Error in CPL_transform(x, crs, aoi, pipeline, reverse, desired_accuracy, : 
crs not found: is it missing?

Running create_c19r_data gives needless output:

> create_c19r_data(df_in = US, asc_bias_list = cbind(AB4 = US$AB))
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"
Joining, by = "geoid"

Additionally, the README.md has a needlessly clumsy construct:

GLOBALMAP <- LoadCountries()
GLOBALMAP <- tidy_Data(GLOBALMAP) 

I see no point to bother a user to call tidy_Data. Instead, I'd encourage either that the LoadCountries function calls tidy_Data in its final setup, or that other functions such as PerCapitaMap_leaflet detect if the data data needs to be tidied and do so if needed.

Regular functions

LoadNetherlands gives needless output:

> Netherlands <- LoadNetherlands()
Reading layer `OGRGeoJSON' from data source 
  `https://geodata.nationaalgeoregister.nl/cbsgebiedsindelingen/wfs?request=GetFeature&service=WFS&version=2.0.0&typeName=cbs_gemeente_2022_gegeneraliseerd&outputFormat=json' 
  using driver `GeoJSON'
Simple feature collection with 345 features and 5 fields
Geometry type: MULTIPOLYGON
Dimension:     XY
Bounding box:  xmin: 13565.4 ymin: 306846.2 xmax: 278026.1 ymax: 619352.4
Projected CRS: Amersfoort / RD New
Rows: 143964 Columns: 12                                                                
── Column specification ──────────────────────────────────────────────────────────────────
Delimiter: ";"
chr  (7): Municipality_code, Municipality_name, Province, Security_region_code, Securi...
dbl  (3): Version, Total_reported, Deceased
dttm (1): Date_of_report
date (1): Date_of_publication

ℹ Use `spec()` to retrieve the full column specification for this data.
ℹ Specify the column types or set `show_col_types = FALSE` to quiet this message.

LoadSweden gives needless output:

> Sweden <- LoadSweden()
trying URL 'https://fohm.maps.arcgis.com/sharing/rest/content/items/b5e7488e117749c19881cce45db13f7e/data'
Content type 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' length 2865276 bytes (2.7 MB)
==================================================
downloaded 2.7 MB

The example moveFiles creates and moves a file in the user's filesystem, without deleting it. This would go against CRAN guidelines, as it comes across as needlessly ignorant of the user (creating useless new files, that is). I encourage the authors to deleted the moved file.

Note that moveFiles has an unclear name (moveProcessedFiles already seems better) and does overwrite files if these are at the target location. I encourage the authors to give an error if there is a file at the target folder that is to be overwritten.

Additionally, what does a function like moveFiles do in a file called addNewGeom? I encourage the authors to either put it into a files called MoveFiles.R or FileUtils.R.

addNewGeoms has an example that is dontrun for some reason. Running it takes no time at all and it does not seem to do anything. I would enjoy to see an example of addNewGeoms that actually does something.

resetNewGeoms does something that seems to not take time, yet it is in dontrun. I am unsure what it does, but whatever it does, it can just be run. I encourage the authors to remove the dontrun.

The calc_risk function has an example function with needless dontrun and -probably due to that- a typo in the call to the function:

#' @examples
#' \dontrun{
#' risk <- calcrisk(.001, 50)
#' }
#'

I suggest to remove the dontrun and call the function with the correct name.

The example from create_c19r_data (except for the needless warnings) does not convey what it does by its return type. As it creates a CSV file, I would encourage the authors to either return the CSV filename or the content of the CSV table.

The example of estRisk is needlessly put into dontrun.

The examples of EventMap_leaflet and EventMap_tmap give an error that does not help the user to find the problem:

> Austria <- LoadAustria()
Error in open.connection(3L, "rb") : 
error:0A00018A:SSL routines::dh key too small

Neither PerCapitaMap_leaflet nor PerCapitaMap_tmap have an example. Please add one :-)

The remSurplus example seems to run in less than 1 second. I suggest to remove the dontrun tag due to that.

I could not check tidy_Data example, as the first line gave en error (see below). I encourage the authors to use an easier country to demonstrate the tidy_Data function.

> Philippines <- LoadPhilippines()
Auto-refreshing stale OAuth token.
Error in `gargle::response_process()`:
! Client error: (403) Forbidden
Insufficient Permission: Request had insufficient authentication scopes.
• domain: global
• reason: insufficientPermissions
• message: Insufficient Permission: Request had insufficient authentication scopes.
Run `rlang::last_error()` to see where the error occurred.

Yes.

There are automated tests that test the package to some extent.

For example, the package is tested to be built. Note that there are many notes/warnings that are ignored in the error logs, e.g. the latest error log at https://github.com/sjbeckett/localcovid19now/actions/runs/3143906398/jobs/5109223012#step:5:304 (needs a GitHub login) has 4 notes. This comes across as needlessly ignorant and I encourage the authors to fix all these.

The main functionality is, however, not tested.

It is understandable that the main functionality is not tested in the code examples in the documentation: these take too long (CRAN uses 5 secs as the definition of 'too long').

However, I would expect another GitHub Actions script that

I do not expect tests about actually using Leaflet or TMap's user interfaces, nor using the data behind the Google Authentication.

But creating an image for easily accessible countries seems the core functionality to me. Put that in a simple R script and add it to the tests.

Yes.

Software paper

Yes, I think a diverse audience can understand what the package does.

Yes

No, not explicitly. Just add it :-)

Yes.

Yes.

Yes, I would agree to accept it as-is. Good job!

As a minor note, I would enjoy to see pictures in the paper. localcovid19now is about putting the spread of COVID-19 into pretty maps. I would strongly expect at least one such picture!

I believe so.

richelbilderbeek commented 2 years ago

@galessiorob I am done reviewing :-)

sjbeckett commented 2 years ago

Hi @galessiorob,

Many thanks for moving this forward! Sorry for my delay here. In terms of other reviewers, here are a few potential suggestions – perhaps yidan-yang, epiben, or stmcg might be interested in reviewing here?

galessiorob commented 2 years ago

Whoa, thank you so much for your review @richelbilderbeek - I'll go ahead and open the review issue and transfer your review over there. We can continue the review in that issue.

@sjbeckett I'll reach out to the candidates you shared and some additional ones.

galessiorob commented 2 years ago

@editorialbot start review

editorialbot commented 2 years ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/4898.