Closed editorialbot closed 1 year ago
editorialbot
commented
1 year ago Hello humans, 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 commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.27 s (1163.3 files/s, 139357.5 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
TeX 1 40 0 239
Markdown 3 144 0 237
YAML 5 29 11 147
SVG 1 0 1 11
Rmd 1 16 26 5
-------------------------------------------------------------------------------
SUM: 319 5612 2852 29751
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 969
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1038/s41562-020-0884-z is OK
- 10.1098/rsif.2010.0142 is OK
- 10.1016/S0140-6736(22)00484-6 is OK
- 10.1038/s41597-021-00955-2 is OK
- 10.1073/pnas.2009911117 is OK
- 10.1038/s41562-020-01000-9 is OK
- 10.21105/joss.03290 is OK
- 10.1073/pnas.2100970118 is OK
- 10.1038/s43587-021-00095-7 is OK
- 10.31234/osf.io/v8tdf is OK
- 10.1038/s41597-022-01245-1 is OK
- 10.1016/S1473-3099(20)30120-1 is OK
- 10.1590/SciELOPreprints.362 is OK
- 10.1503/cmaj.75262 is OK
- 10.1016/S2468-2667(20)30225-5 is OK
- 10.21105/joss.02376 is OK
- 10.18637/jss.v084.i06 is OK
MISSING DOIs
- None
INVALID DOIs
- None
galessiorob
commented
1 year ago @richelbilderbeek unfortunately GitHub doesn't have a feature to transfer comments to existing issues so I'll just copy-paste the original one here so that we can continue the review.
FROM @richelbilderbeek
Here is my review! See https://github.com/richelbilderbeek/review_localcovid19now to see it in plaintext :-)
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.
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 |
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
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 NARunning 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.
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 MBThe 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 smallNeither 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.
Yes, I think a diverse audience can understand what the package does.
A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
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.
galessiorob
commented
1 year ago @sjbeckett please go over @@richelbilderbeek review comments at your earliest convenience. I'll add new reviewers once they accept the invite to review the paper.
galessiorob
commented
1 year ago @editorialbot add @mbkumar as reviewer
editorialbot
commented
1 year ago @mbkumar added to the reviewers list!
galessiorob
commented
1 year ago @mbkumar thank you so much for volunteering to review this paper! Please comment @editorialbot generate my checklist to review your checklist and let me know if you have any questions on how to proceed.
mbkumar
commented
1 year ago galessiorob
commented
1 year ago @editorialbot add @welch16 as reviewer
editorialbot
commented
1 year ago @welch16 added to the reviewers list!
galessiorob
commented
1 year ago @welch16 thank you so much for volunteering to review this paper! Please comment @editorialbot generate my checklist to review your checklist and let me know if you have any questions on how to proceed.
galessiorob
commented
1 year ago @editorialbot add @epic19mz as reviewer
editorialbot
commented
1 year ago @epic19mz added to the reviewers list!
galessiorob
commented
1 year ago @Epic19mz thank you so much for volunteering to review this paper! Please comment @editorialbot generate my checklist to review your checklist and let me know if you have any questions on how to proceed.
Epic19mz
commented
1 year ago @editorialbot generate my checklist
editorialbot
commented
1 year ago @Epic19mz I can't do that because you are not a reviewer
galessiorob
commented
1 year ago @arfon ran into the same issue with editorialbot, not sure what's causing the glitch.
galessiorob
commented
1 year ago @editorialbot add @Epic19mz as reviewer
editorialbot
commented
1 year ago @Epic19mz added to the reviewers list!
galessiorob
commented
1 year ago @Epic19mz mind trying again, please?
Epic19mz
commented
1 year ago @galessiorob I think it is now a good time to share my comments below to the authors. Thanks!
This is a great work which provides a convinent way for people fetching the latest Covid-19 data in R. The breath of Covid data included in this package is very impressive. It also supports some visualization methods in a global scale. I do believe this work is worth to publish, but there could be some improvements (see my comments below) before finial acceptance.
[x] Repository: Is the source code for this software available at the https://github.com/sjbeckett/localcovid19now? Yes.
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license? MIT license.
[x] Contribution and authorship: Has the submitting author (@sjbeckett) made major contributions to the software? Does the full list of paper authors seem appropriate and complete? Yes, the submitting author made major contributions. Most Github contributors are listed as authors, but it is not clear to me why Kaitlyn Johnson who submitted 12,330 lines isn't in the author list.
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines Yes, based on factors provided by JOSS: Age of software (is this a well-established software project) / length of commit history: 1 year. Number of commits: 278. Number of authors: 5. Total lines of code (LOC): 29751. (Submissions under 1000 LOC will usually be flagged, those under 300 LOC will be desk rejected.) Whether the software has already been cited in academic papers: Not yet. Whether the software is sufficiently useful that it is likely to be cited by your peer group: Yes. This software provides a simple way to fetch recent Covid-19 data and provides some helpful visualization methods.
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.
[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed? Most of them work well, except the followings:
Some load functions give a summary of fetched data, but some load functions don't return that summary. For example, LoadAustralia() gives a nice summary below, but LoadAlgeria() doesn't give any summary.
> Algeria <- LoadAlgeria()
> Australia <- LoadAustralia()
Rows: 8120 Columns: 12
── Column specification ──────────────────────────────────────────────────
Delimiter: ","
chr (2): state, state_abbrev
dbl (9): confirmed, deaths, tests, positives, recovered, hosp, icu, v...
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.Although the function LoadAustralia() claims to fectch a data with 8120 rows and 12 columns, but implementing the function View(Australia) gives a dataframe with 8 rows and 6 columns.
It is not clear to me how to use the function spec() since this function cannot be found.
It would be better if the function LoadAustralia() allows user to choose a directory to save the data.
LoadBelgium() returns warnings without further explanations in documents. Similar things have happened to LoadSouthKorea().
LoadGermany() returns many blocks of data summary. It would be great to specify the differences between them or just return one block for data summary if each block is part of the entire fetched data.
Functions LoadItaly(), LoadNewZealand(), don't work for me. As an example,
> LoadItaly()
Error in open.connection(3L, "rb") : HTTP error 404.[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
Most functions do have examples except for PerCapitaMap_leaflet and PerCapitaMap_tmap. I do recommend to add some example for them. Moreover, it would be better to add some examples for the function LoadCountries with the argument countries is not NULL. For example, I run the following codes to fectch data for US and Malaysia, but somehow the function PerCapitaMap_leaflet doesn't work well with its output.
> GLOBALMAP <- LoadCountries(c('US','Malaysia'))
US
Malaysia
> GLOBALMAP <- tidy_Data(GLOBALMAP)
> #show per capita active cases per 100,000 people in leaflet
> PerCapitaMap_leaflet(GLOBALMAP,100000)
Error in polygonData.default(data) :
Don't know how to get path data from object of class listPerCapitaMap_tmap, PerCapitaMap_leaflet, EventMap_tmap, EventMap_leaflet seems incomplete.
The description of the package localcovid19now seems incomplete.LoadCountries, tidy_Data, PerCapitaMap_leaflet, PerCapitaMap_tmap, EventMap_leaflet, EventMap_tmap, create_c19r_data.
That's very helpful.Epic19mz
commented
1 year ago @galessiorob Thanks for your help! It works now!
welch16
commented
1 year ago welch16
commented
1 year ago I experienced a minor issue installing the package using devtools::install_github. I added the following issue:
galessiorob
commented
1 year ago @editorialbot add @carinogurjao as reviewer
editorialbot
commented
1 year ago @carinogurjao added to the reviewers list!
galessiorob
commented
1 year ago @sjbeckett 👋 please start addressing the comments and issues noted by the reviewers at your earliest convenience so we can get your paper ready for publishing. Let me know if you have any questions or are blocked in any way.
@welch16 @Epic19mz and @richelbilderbeek Thank you so much for your review so far!
@mbkumar let me know if you are blocked in any way from making progress on your review.
@carinogurjao thanks for acting as a reviewer for this submission!
sjbeckett
commented
1 year ago Thanks @galessiorob, we are starting to address. Will be in touch!
carinogurjao
commented
1 year ago mbkumar
commented
1 year ago @galessiorob I took the first pass at the code. I updated my checklist with my comments. Once the issues raised are addressed, I will take another look.
welch16
commented
1 year ago Added an issue suggesting the use of unit tests
https://github.com/sjbeckett/localcovid19now/issues/62
According to the JOSS documentation, the github automated actions would be enough to check the automated tests checkbox, but I think that including unit tests may improve the package.
galessiorob
commented
1 year ago Checking in 🎉
@welch16 and @mbkumar thanks again for the review and the improvement recommendations! @welch16 I agree that including unit tests would be a good addition on top of GH Actions.
@sjbeckett please let me know when this is ready for another full review - I hope we can this paper published soon!
sjbeckett
commented
1 year ago Many thanks @galessiorob. We are getting close and plan to merge revisions into our main branch and send a response early next week.
We have added a github actions check, and have implemented unit tests for some of our core functions, though we think unit tests would be tricky to implement across the loading functions as we noted in response to @welch16 https://github.com/sjbeckett/localcovid19now/issues/62#issuecomment-1353344286 . More soon!
galessiorob
commented
1 year ago Sounds great! I'll check in soon.
sjbeckett
commented
1 year ago Many thanks to all of you for taking the time to review our package and provide critical feedback -- and thank you @galessiorob for handling the review process. We have spent time working through your feedback and suggestions which we outline below – these edits are now committed and merged into the main branch of sjbeckett/localcovid19now which we have released as version 0.1. A link to the pull request with changes is here: sjbeckett/localcovid19now/pull/67. Our revisions are as follows:
Manuscript:
“Our package aims to facilitate the process of downloading and visualizing recent COVID-19 data, in support of public health research and campaigns to inform and alert the public of the ongoing risk of transmission -- and is of particular interest to public health scientists and researchers, epidemiologists, community leaders, and policymakers.“
We updated reference formatting as suggested by @Epic19mz.
We now incorporate a figure showing an example output as suggested by @richelbilderbeek. We purposefully chose to show a figure that represents the scope of the project, including regions which are not currently reporting recent data (an issue described in the manuscript).
To clarify author contributions we now include a statement in the manuscript. Nguyen's contributions were predominantly pre-GitHub, with contributions to overall data collection methodology, as well as coding up loading functions for several countries including, but not limited to Belgium, Brazil, China, Netherlands, Norway, Japan, Spain. We also note that important contributions came from Rishishwar, Andris and Weitz who provided technical guidance and project management. We include the following author attribution statement:
“SJB led the project. SJB, FB-T, QN, and ATC contributed to dataset collection and coding methods for data curation. SJB, FB-T, and ATC contributed to package development. LR, CA, and JSW provided substantial contributions via decision and design guidance, and project management. SJB wrote the manuscript with contributions from all authors.”
Reviewers noted that Johnson has provided contributions to our project. We appreciate Johnson’s contribution to patching our LoadCanada function when the location of the datasource changed. Johnson is acknowledged in our manuscript, and we have added Johnson as a contributor “ctb” within our DESCRIPTION file, following: https://stat.ethz.ch/R-manual/R-devel/library/utils/html/person.html.
README and Examples:
We have updated the example documentation in the README.md and corresponding examples directory as noted by @mbkumar here: sjbeckett/localcovid19now/issues#59
We also provided more information about using the googledrive library (needed for LoadPhilippines) as suggested by @richelbilderbeek, which will not work unless certain permissions are accepted by the user – even if a googledrive token is acquired! We added a check within LoadPhilippines to check if users have supplied permissions such that the function can run via googledrive::drive_about(), and returned a more user-friendly error message (than the gargle permissions error) with instructions on how users can update permissions and authorization by running googledrive::drive_auth().
We noted that users may need to load up packages that our package imports from as suggested by @mbkumar. Hopefully this will be seen as more user friendly – and we do sympathize as we have also previously faced these issues using R – but, as these challenges in installation are rooted in other packages, we believe this goes beyond the scope of our package. We note for completeness that the packages within the R ecosystem that are used within our package are listed in the DESCRIPTION file; which is used by R’s package management system to automate installation/loading.
We also added a note regarding the possibility of changing timeout options in helping users to install the package as found by @welch16 in the issue raised: sjbeckett/localcovid19now#58
Code Base:
We fixed data source file loading for New Zealand and Italy, which were previously failing.
We suppressed progress, summary, and console out messages within data loading calls (and create_c19r_data) following advice from @richelbilderbeek. While we agree some of the prior summary output (via the vroom package) is nice as noted by @Epic19mz we preferred this approach as it reduces verbosity and provides more cohesion across different data loading calls (not all data is loaded via vroom due to differences in underlying file types/storage).
We updated package documentation (including PerCapitaMap_tmap, PerCapitaMap_leaflet, EventMap_tmap, EventMap_leaflet) inline with suggestions from @Epic19mz and @richelbilderbeek and removed many instances of dontrun across the package. Some instances of dontrun remain, as some functions take >5s to run.
R CMD checks are run on submitting pull requests, and we have worked to fix and remove warnings and notes that were previously generated by these checks by removing .gitkeep files, and moving nonstandard files/folders into the inst folder or adding them to the .Rbuildignore list.
We revisited how tidy_Data was being applied within the package. We now encourage users to use the function LoadData, which performs both loading and tidying of the dataset (using the tidy_Data). i.e. we now instruct users to run LoadData(“LoadUS”) rather than LoadUS(). We have updated examples in the README to reflect this. Additionally to help usability, we have included some alternative matchings to help users i.e. the calls LoadData(“USA”), LoadData(“LoadUS”) and LoadData(“United States”) all produce equivalent output. A list of main function calls are stored in countrylist, with a list of alternate matchings stored in alt_countrylist.
We added checking of user inputs within LoadData, such that user inputs are of the correct type and contain appropriate values, with informative error messaging if not, which should improve the usability of our package.
We note that some functionality within our package is geared more towards development purposes than for a typical user. At the same time, we believe these files are more useful if they are kept with the package than removed/moved elsewhere. As such, we have made the functions previously within files: addnewGeom.R,updateGlobal.R, remSurplus.R internalised functions, and moved them together into a new file geomUpdateUtils.R. Additionally, as suggested by @richelbilderbeek we renamed the function moveFiles to moveProcessedFiles.
Automated tests:
Following @richelbilderbeek’s suggestion we introduced a new github action check (in addition to R CMD checks) to test package functionality by attempting to download all data files and render them via PerCaptiaMap_tmap following a pull request.
@welch16 had a great recommendation for incorporating unit tests. We chose to implement unit tests for some common utility functions used by our package, namely tidy_Data, assertOptions__tidy_data, estRisk and calcRisk. However, given the dynamic nature of the data the loading functions ingest, the output is non-deterministic which makes unit testing significantly harder to implement across the entire package. Alternatively, we could break each function into smaller units but that would entail a complete rewrite of the code to accommodate unit tests. In lieu of this, we have added additional failsafes to improve user experience and make the package more rigorous. In particular, we have implemented tryCatch loops within LoadData to better handle any potential errors from any of the particular data source loads. We note our expectation is that many of the underlying datasets are not going to be stable resources in the long-term (which is ok!).
Other comments:
We note to @Epic19mz that our focus in the package is on recent data, but some data resources are saved using the entire timeseries (e.g. LoadAustralia), while others have different files corresponding to data on individual days (e.g. LoadBelgium). Such differences were a key motivator in developing this project.
We additionally updated code for Canada following changes in source highlighted in a new issue outside of the context of this review: sjbeckett/localcovid19now#63
There are some comments we have not been able to fully resolve. In particular, while we are able to run LoadAustria and LoadDenmark locally, they seem to not run within the automated ubuntu environment we set up to generate maps. We have also been unable to find a solution to prevent warning errors regarding closing unused connections from appearing in the console. These errors correspond to attempting to open invalid URL’s as we search for recent data (date is encoded in the URL string) within the LoadBelgium and LoadMexico functions.
Again, thank you all for your feedback leading to these revisions. Please reach out if you have further questions.
welch16
commented
1 year ago Everything looks good on my end. Really cool work @sjbeckett,
Happy holidays everyone!
galessiorob
commented
1 year ago @carinogurjao 👋 checking in! This paper is very close to being ready to publish. Are you still able to add your review? Please let me know; otherwise, I'll have to remove you as a reviewer to avoid delaying the publishing. Thanks!
galessiorob
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Epic19mz
commented
1 year ago @sjbeckett I don't have further questions from my end! Great work! Congratulation!
galessiorob
commented
1 year ago @sjbeckett amazing work! I just did a final run looking at the latest changes, and all looks good 🚀
I put together a few suggestions for the actual paper in this branch from a fork. They are minor, please incorporate them as you see fit.
After that, I think we should be ready to publish.
richelbilderbeek
commented
1 year ago Well done and thanks :+1: ! Enjoy the glory from the publication!
sjbeckett
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
sjbeckett
commented
1 year ago @sjbeckett amazing work! I just did a final run looking at the latest changes, and all looks good 🚀
I put together a few suggestions for the actual paper in this branch from a fork. They are minor, please incorporate them as you see fit.
After that, I think we should be ready to publish.
Many thanks @galessiorob. I have incorporated your suggestions for the manuscript this morning. Excited to move forward!
galessiorob
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
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: v0.1 Editor: !--editor-->@galessiorob<!--end-editor-- Reviewers: @richelbilderbeek, @mbkumar, @welch16, @Epic19mz Archive: 10.5281/zenodo.7542478
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@richelbilderbeek, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @galessiorob know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @mbkumar
📝 Checklist for @welch16
📝 Checklist for @Epic19mz
📝 Checklist for @Epic19mz
📝 Checklist for @Epic19mz
📝 Checklist for @Epic19mz
📝 Checklist for @carinogurjao