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
Briefly describe any working relationship you have (had) with the package authors.
[x] 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:
[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[x] Examples: (that run successfully locally) for all exported functions
[ ] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
Review Comments
On statement of need
Package has a README that details why the software was developed and what needs it fills/addresses. However, the background on why the tool was developed (IPC AMN) and the rules (SMART) and tooling (nipnTK) the package is built on is not clearly expounded. I think it is important to state here that the package being reviewed primarily uses already existing functionalities from another package (nipnTK) and what the current package primarily does is provide a wrapper to these functions to implement checks based on SMART rules and how IPC performs its data quality checks. As such, the package name {ipccheckr} is a bit misleading as the checks performed are not really guidelines set by IPC but rather an implementation of rules set by SMART and using functions/tools provided by {nipnTK}. Hence, a more appropriate name should signify/indicate such. A name that alludes to the SMART origins of the rules might be more appropriate (smartchecks) or a name that indicates that this is primarily a tool for reporting on anthropometric data - so maybe something like anthroporter short for anthropometric reporter. The point of the comment basically is that the name is not compatible with the purpose stated and the problems that are being addressed.
In terms of coherence of software purpose to actual functionality, why add functions for prevalence estimation when this is meant to be a checker? I think this is an inconsistency and prevalence estimation is out of scope.
In relation to the statement of need are specific feedback conerning the README text and the README code/implementation:
in the Rmd setup code chunk, you used devtools::install_github("tomaszaba/ipccheckr"). You shouldn't add code like this on any script especially in an Rmd. The decision to install should be the option of the user. Locally, you should install the package using devtools::install() before knitting the README.
Too much use of specialist jargon and acronyms/abbreviations (e.g., IPC AMN, plausibility checks) and use of technical shorthand (e.g., 6:23 instead of 6 to 23 months, etc.).
Need to check grammar and sentence construction. A lot of sentences are too long and sometimes incoherent.
Workflow using DiagrammeR is clever but the diagram doesn't really look nice/good; alignment is off; using grViz requires/forces you to specify always_allow_html: true which then messes up github markdown such that your callout-tip is now working properly. I suggest using mermaid instead and using mermaid syntax directly rather than wrapping it in R using DiagrammeR.
the hex sticker is too low; the recommended is to align it to the main heading.
add badge for repostatus (set to WIP - work in progress) and badge for lifecycle (set to experimental)
CSS style for callout-tip is showing in the HTML output. This shouldn't be the case. The HTML implementation is getting confused with your use of the grViz from DiagrammeR.
instructions on how to install should be somewhere at the start right after the what does ipccheckr do.
use remotes::install_github() rather than devtools::install_github() - remotes uses a lot less dependency than devtools; in Windows, installing devtools require installing developer tools which will be much complicated for regular R users.
There is no contributing guidelines and there is no community guidelines. These are important.
Action points for README:
[ ] #58
[ ] #40
[ ] #41
[ ] #42
[ ] #43
[ ] #44
[ ] #45
[ ] #46
[ ] #47
[ ] #48
[ ] #49
[ ] #50
Vignettes
like the README, vignettes are too wordy and a bit incoherent
when setting css: style.css in yaml, you need to create a style.css file as well
doesn't make sense to put a dynamic date in vignettes - date: "r format(Sys.Date(), '%B %d, %Y')" - even if you didn't change the vignette, everytime you deploy the package to pkgdown for every new R-CMD-check, the vignette will be re-rendered and with a new date will seem like it has been updated. Either set a fixed date and the change when you edit/change vignette or don't put a date at all.
Action points for vignettes:
[ ] #51
[ ] #52
[ ] #53
Function documentation
function documentation has descriptions that are too long!! Descriptions should be more succinct and simple. If added notation is needed/wanted, then this should be added as detail and with sections that you can refer to. These notes get added after the documentation about the parameters. This is more readable in that users can see syntax first/right away before any detail. Right now, the user is bombarded with very long text even before he/she can see the syntax.
the roxygen notation is very chaotic and not human-readable. I prefer that each argument has its own @param line rather than combinations. I find this much harder to read as a user when looking at help files.
the roxygen notation for example code is very chaotic. Treat this in the same way as if you were coding as a script.
Action points for function documentation:
[ ] #54
[ ] #55
[ ] #56
Other documentation feedback/action points
on devtools::check() I see:
< Potential spelling errors:
< WORD FOUND IN
< Bassa prevalence.Rmd:225
< Cahora prevalence.Rmd:225
< Chiuta prevalence.Rmd:225
< Maravia prevalence.Rmd:183,225
< Metuge prevalence.Rmd:183,225
< Oftentimes README.md:30
< R's plausiblity.Rmd:27
< sample_size.Rmd:37
< ipc sample_size.Rmd:67
< obs sample_size.Rmd:67
< plausibile prevalence.Rmd:102
< requeriments sample_size.Rmd:2
< zscorer plausiblity.Rmd:110
< ️ README.md:117
< If these are false positive, run `spelling::update_wordlist()`.All Done!
I see about 3 of these words needing spelling correction
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:
URL,BugReportsandMaintainer(which may be autogenerated viaAuthors@R).Review Comments
On statement of need
Package has a README that details why the software was developed and what needs it fills/addresses. However, the background on why the tool was developed (IPC AMN) and the rules (SMART) and tooling (nipnTK) the package is built on is not clearly expounded. I think it is important to state here that the package being reviewed primarily uses already existing functionalities from another package (nipnTK) and what the current package primarily does is provide a wrapper to these functions to implement checks based on SMART rules and how IPC performs its data quality checks. As such, the package name
{ipccheckr}is a bit misleading as the checks performed are not really guidelines set by IPC but rather an implementation of rules set by SMART and using functions/tools provided by{nipnTK}. Hence, a more appropriate name should signify/indicate such. A name that alludes to the SMART origins of the rules might be more appropriate (smartchecks) or a name that indicates that this is primarily a tool for reporting on anthropometric data - so maybe something likeanthroportershort for anthropometric reporter. The point of the comment basically is that the name is not compatible with the purpose stated and the problems that are being addressed.In terms of coherence of software purpose to actual functionality, why add functions for prevalence estimation when this is meant to be a checker? I think this is an inconsistency and prevalence estimation is out of scope.
In relation to the statement of need are specific feedback conerning the README text and the README code/implementation:
in the Rmd setup code chunk, you used
devtools::install_github("tomaszaba/ipccheckr"). You shouldn't add code like this on any script especially in an Rmd. The decision to install should be the option of the user. Locally, you should install the package usingdevtools::install()before knitting the README.Too much use of specialist jargon and acronyms/abbreviations (e.g., IPC AMN, plausibility checks) and use of technical shorthand (e.g., 6:23 instead of 6 to 23 months, etc.).
Need to check grammar and sentence construction. A lot of sentences are too long and sometimes incoherent.
Workflow using DiagrammeR is clever but the diagram doesn't really look nice/good; alignment is off; using grViz requires/forces you to specify
always_allow_html: truewhich then messes up github markdown such that your callout-tip is now working properly. I suggest using mermaid instead and using mermaid syntax directly rather than wrapping it in R using DiagrammeR.the hex sticker is too low; the recommended is to align it to the main heading.
add badge for repostatus (set to WIP - work in progress) and badge for lifecycle (set to experimental)
CSS style for callout-tip is showing in the HTML output. This shouldn't be the case. The HTML implementation is getting confused with your use of the grViz from DiagrammeR.
instructions on how to install should be somewhere at the start right after the what does ipccheckr do.
use
remotes::install_github()rather thandevtools::install_github()- remotes uses a lot less dependency than devtools; in Windows, installing devtools require installing developer tools which will be much complicated for regular R users.There is no contributing guidelines and there is no community guidelines. These are important.
Action points for README:
Vignettes
css: style.cssin yaml, you need to create astyle.cssfile as wellr format(Sys.Date(), '%B %d, %Y')" - even if you didn't change the vignette, everytime you deploy the package to pkgdown for every new R-CMD-check, the vignette will be re-rendered and with a new date will seem like it has been updated. Either set a fixed date and the change when you edit/change vignette or don't put a date at all.Action points for vignettes:
Function documentation
function documentation has descriptions that are too long!! Descriptions should be more succinct and simple. If added notation is needed/wanted, then this should be added as detail and with sections that you can refer to. These notes get added after the documentation about the parameters. This is more readable in that users can see syntax first/right away before any detail. Right now, the user is bombarded with very long text even before he/she can see the syntax.
the roxygen notation is very chaotic and not human-readable. I prefer that each argument has its own @param line rather than combinations. I find this much harder to read as a user when looking at help files.
the roxygen notation for example code is very chaotic. Treat this in the same way as if you were coding as a script.
Action points for function documentation:
Other documentation feedback/action points
devtools::check()I see:I see about 3 of these words needing spelling correction