JOSS Review: @jackmwolf

[jackmwolf]

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."

[ldliao]

@jackmwolf Thank you so much for taking the time to review this package.
I have added all the requested items and edits to the one_hot branch
Here is a point by point update in response to your review:

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.

• Thank you for your feedback and I had made changes to the code. This is the largest change within the new version -- where instead of requiring indicator variables for the categorical variables (as suggested), I wrote code (and tested) to convert character variables into factor variables and do one-hot encoding. Since the treatment variable is explicitly either 0 or 1, I added in the documentation to specify the outcome variable must be a single numeric variable. For the conversion, the binary variables default selects the first level as 1 and other level as 0. Since for multi-level factor variables might have yield variables that are not present in both data sets, the code explicitly selects the variables that are present in both.

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.

• Thank you for noting this and I have updated accordingly 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.

• Thank you for your suggestion! I ended up using a different documentation than the suggested example but a CONTRIBUTING.md file is now present.

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.

• Thank you for bringing this to my attention. I have deleted the LICENSE.md file and input the software license information into LICENSE file.
• Edit: the LICENSE and LICENSE.md is set up back to how the package set up is typically generated. I brought this back in for the LICENSE and LICENSE.md information and updated the copy right years.

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

• Thank you for encouraging me to expand on the statement. I have updated this to the following text:
  "Bias curves enable comparisons to support variable prioritization among potential confounders."

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

• Thank you for catching this typo, it has been edited accordingly.

[jackmwolf]

Hi again, @ldliao! All of these changes look great, I'm happy with the state of the software and paper and will update my review in openjournals/joss-reviews#6093. Please merge this branch into main before issuing the package release that will be included with the JOSS paper. Thanks!

[ldliao]

Hi @jackmwolf thanks for taking the time for the review! I just merged the one_hot branch into main (and updated the news to reflect the latest updates!