ldliao / jointvip Goto Github PK

Hi @ldliao! I'm done with my initial review of jointVIP for openjournals/joss-reviews/issues/6093. The functionality and purpose are great overall. I've listed several minor suggestions for improvement below:

1. Based on the provided examples and my own testing, it appears that create_jointVIP() does not support categorical variables with >2 levels and such variables must be converted into a set of indicator variables first. I suggest being more explicit about the input requirements in the documentation for create_jointVIP(). Currently, the only listed requirements are that pilot_df and analysis_df are data.frames.

2. Similarly, the create_jointVIP() returns an error if treatment is not binary (e.g., if it is coded as the strings 'treatment' and 'control'). This expectation could be made explicit in the documentation.

3. Consider adding documentation for how others can contribute to your software somewhere in your root directory (e.g., CONTRIBUTING.md). You can find an example here.

4. The GitHub repository contains both LICENSE and LICENSE.md. The former only lists the year and copyright holder and is not a software license.

5. Can you expand on the statement that "[b]ias curves enable comparisons to support prioritization" in README.md? What is prioritized?

6. (Very minor) Line 62 of paper.md says "functionto" instead of "function to."

Code/Repo

• Note that the example in the README doesn't work immediately when copy-pasted into an R session (I often do this, presumably others too) because the object df used in the example is created in a hidden Rmd code block, which downloads a CSV of a dataset that is then cleaned to produce df. A cleaner solution would be to move this hidden block to a standalone R script (stored in the package) and distribute the cleaned dataset as part of your package, as described at https://r-pkgs.org/data.html; this has the advantage of making the package self-contained wrt data, as changes in the availability of that CSV won't affect this package's examples/demos.
• Code organization: Most of the package's core functions seem to be organized in a file general.R, which is fine but may prove challenging to work with for contributors. It's recommended that functions be stored in individual files or at least thematically organized (e.g., classes.R to store your S3 class definitions); see https://r-pkgs.org/code.html#sec-code-organising.
• The suite of unit tests seems to be reasonably diverse but it's hard to get a sense without a code coverage check in place. On a related note, GitHub Actions seems to be activated but is only being used to produce the JOSS PDF. A better use of the Actions workflow would be to automate the running of unit tests (activated on push and PRs) and code coverage checks; some examples of actions for R are at https://github.com/r-lib/actions
• Per JOSS review guidelines, some statement about community guidelines is necessary. This could be either as a separate markdown file in the repo or a new section in the existing README file.

Paper

• There's a minor typo in the abstract (line 14 of JOSS PDF): "The joint variable importance plot (jointVIP) package to guides decisions about which variables to prioritize for adjustment by quantifying and visualizing each variable's relationship to both treatment and outcome" (emphasis mine).
• On "Development" (lines 49-50 of JOSS PDF): It would be clearer to note that the package exposes a new S3 class, called jointVIP, and exposes methods of the generic functions print(), summary(), and plot() for this class (as opposed to saying that it "leverages system generic functions").
• On "Usage", a few typos: "outcomeis" instead of "outcome is" and "functionto" instead of "function to"