search

openjournals / joss-reviews

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

[REVIEW]: surtvep: An R package for estimating time-varying effects #5688

Closed editorialbot closed 4 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@LingfengLuo0510<!--end-author-handle-- (Lingfeng Luo) Repository: https://github.com/UM-KevinHe/surtvep Branch with paper.md (empty if default branch): JOSS Version: v1.0.0 Editor: !--editor-->@osorensen<!--end-editor-- Reviewers: @adibender, @turgeonmaxime Archive: 10.5281/zenodo.12575049

Status

status

Status badge code:

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

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

@adibender & @turgeonmaxime, 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:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @osorensen 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 @turgeonmaxime

📝 Checklist for @adibender

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 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 1 year ago

Checking the BibTeX entries failed with the following error: 

Failed to parse BibTeX on value "title" (NAME) [#<BibTeX::Bibliography data=[8]>, "@", #<BibTeX::Entry >, "%"]
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.10 s (720.9 files/s, 190900.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                            31           1137            263           6636
C++                              3            310            446           2410
XML                              2              0            133           1877
R                               14            231           1458           1485
CSS                              3             98             52            442
JavaScript                       3             64             32            256
TeX                              2             29              9            225
Markdown                         4             91              0            215
Rmd                              5            302            617            113
YAML                             3              6              2            109
SVG                              1              0              1             11
C/C++ Header                     1              2              0              5
-------------------------------------------------------------------------------
SUM:                            72           2270           3013          13784
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1759

editorialbot commented 1 year ago

Failed to discover a valid open source license

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:

turgeonmaxime commented 1 year ago

Review checklist for @turgeonmaxime

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

adibender commented 1 year ago

Review checklist for @adibender

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

adibender commented 1 year ago

@osorensen I work in the field and have a package which provides similar functionality. Not sure if this is conflict of interest, but I don't think it will affect review. Just wanted to mention it here

osorensen commented 1 year ago

@adibender, I agree. I see this as a strength rather than a conflict of interest, but thanks for mentioning it.

turgeonmaxime commented 1 year ago

@LingfengLuo0510 I just read the manuscript for the first time, and I have a few questions/comments.

I'm sure I'll have further comments/questions as I keep reviewing the package, but I'll put them in a different comment!

turgeonmaxime commented 1 year ago

Also, I haven't been able to install the package using the instructions in the manuscript. Here is the error message I got.

r$> remotes::install_github('UM-KevinHe/surtvep')
Downloading GitHub repo UM-KevinHe/surtvep@HEAD
── R CMD build ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   checking for file ‘/private/var/folders/hb/r5l2r5b12gg5y2zxyg4l7nh80000gn/T/Rtmp42cwos/remotes1030ea884ab0/UM-KevinHe-surtvep-31f9fd4/DESC✔  checking for file ‘/private/var/folders/hb/r5l2r5b12gg5y2zxyg4l7nh80000gn/T/Rtmp42cwos/remotes1030ea884ab0/UM-KevinHe-surtvep-31f9fd4/DESCRIPTION’
─  preparing ‘surtvep’:
✔  checking DESCRIPTION meta-information ...
─  cleaning src
─  checking for LF line-endings in source and make files and shell scripts
─  checking for empty or unneeded directories
─  building ‘surtvep_1.0.0.tar.gz’

* installing *source* package ‘surtvep’ ...
** using staged installation
** libs
using C++ compiler: ‘Apple clang version 14.0.3 (clang-1403.0.22.14.1)’
using C++11
using SDK: ‘MacOSX13.3.sdk’
clang++ -arch arm64 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG  -I'/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Rcpp/include' -I'/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppArmadillo/include' -I/opt/R/arm64/include     -fPIC  -falign-functions=64 -Wall -g -O2  -c PenalizeStopCpp.cpp -o PenalizeStopCpp.o
PenalizeStopCpp.cpp:4:10: fatal error: 'omp.h' file not found
#include <omp.h>
         ^~~~~~~
1 error generated.
make: *** [PenalizeStopCpp.o] Error 1
ERROR: compilation failed for package ‘surtvep’
* removing ‘/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/surtvep’
* restoring previous ‘/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/surtvep’
Warning message:
In i.p(...) :
  installation of package ‘/var/folders/hb/r5l2r5b12gg5y2zxyg4l7nh80000gn/T//Rtmp42cwos/file1030e79df6e36/surtvep_1.0.0.tar.gz’ had non-zero exit status

I did notice that the instructions in the README.md file are slightly different, as they point to the branch openmp. When I follow those instructions (i.e. remotes::install_github("UM-KevinHe/surtvep", ref = "openmp")), then the installation proceeds without error.

For reference, here's my session information:

r$> sessionInfo()
R version 4.3.0 (2023-04-21)
Platform: aarch64-apple-darwin20 (64-bit)
Running under: macOS Ventura 13.4.1

Matrix products: default
BLAS:   /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/lib/libRblas.0.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/lib/libRlapack.dylib;  LAPACK version 3.11.0

locale:
[1] en_CA.UTF-8/en_CA.UTF-8/en_CA.UTF-8/C/en_CA.UTF-8/en_CA.UTF-8

time zone: America/Edmonton
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base

loaded via a namespace (and not attached):
 [1] processx_3.8.2    compiler_4.3.0    R6_2.5.1          rprojroot_2.0.3   cli_3.6.1         prettyunits_1.1.1 tools_4.3.0
 [8] curl_5.0.1        crayon_1.5.2      remotes_2.4.2     desc_1.4.2        callr_3.7.3       ps_1.7.5          pkgbuild_1.4.2
turgeonmaxime commented 1 year ago

@LingfengLuo0510 One more thing: I ran devtools::check() on the openmp branch, and it highlighted many issues with the documentation of your package (among other things). I suggest you ran devtools::check() yourself and fix the warnings, and consider fixing the notes too.

osorensen commented 1 year ago

Thanks a lot for you comments @turgeonmaxime!

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:

osorensen commented 1 year ago

@LingfengLuo0510, I just wanted to add to the comments from @turgeonmaxime that although we don't require R package submissions to be on CRAN, they should in general pass R CMD check with no NOTEs, WARNINGs, or ERRORs, unless there are very good arguments for not doing so. However, since this is an R package that needs compilation, I would also strongly recommend uploading it to CRAN, as it makes the installation process much easier for most users.

osorensen commented 1 year ago

@LingfengLuo0510, you're welcome to start addressing the points raised by @turgeonmaxime. Please keep us updated about any progress.

LingfengLuo0510 commented 1 year ago

@osorensen Thanks a lot for the instructions. I am submitting this project to CRAN now. I will update when it is successfully uploaded. @turgeonmaxime The installation issues come from the fact that OpenMP feature is naturally supported in the Mac OS m1 chip system. I am uploading the package to CRAN and trying to fix the issue. I will reply to the points soon. Thanks a lot for the comments!

adibender commented 1 year ago

@LingfengLuo0510 some things that are currently missing from your package:

osorensen commented 1 year ago

Thanks @adibender.

@LingfengLuo0510, any updates on fixing the issues pointed out be @turgeonmaxime or on the CRAN submission?

LingfengLuo0510 commented 1 year ago

Thank you for your comments. The package is currently under CRAN review. I’ve addressed CRAN's feedback and resubmitted the updated package for approval. I will provide an update once the review process is complete.

osorensen commented 11 months ago

@LingfengLuo0510, I notice the package has been on CRAN for a while now. Could you please let us know if there's also been any progress in addressing the points raised by the reviewers?

editorialbot commented 11 months ago

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

LingfengLuo0510 commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

LingfengLuo0510 commented 11 months ago

@turgeonmaxime

Thanks a lot for your comments.

  1. "I noticed that Figure 1 (the function flowchart) doesn't appear in the PDF. In its place, it looks like the caption is repeated twice. Please fix." Fixed. See the revised Figure 1.

  2. "In Figure 2, the confidence bands don't cover the same range as the curve estimate. Why is that?" The y-axis range in the updated Figure 2 has been expanded to ensure complete coverage of the confidence bands and the curve estimate. See the updates in Figure 2.

  3. "On line 119, you write: "We estimate cancer stage of kidney, lung, and breast." That's incorrect, you're estimating hazard ratios. The cancer stages are part of the data." See the revised text on line 128 for the correct information.

  4. "For the section "Data Example", would it be possible to share your code?" The code for model fitting and plotting can be found in the Repository-joss branch. The name of the file is "GenerateRealDataPlot.R". Access to data can be requested at https://seer.cancer.gov/data/access.html.

  5. "In the manuscript, could you clarify when a user should opt for a penalized approach? Does it depend on the number of covariates, or the number of basis functions?" We have added a detailed paragraph on line 16 to clarify when a user should opt for a penalized approach.

  6. "Also, I haven't been able to install the package using the instructions in the manuscript. Here is the error message I got." The package is now published on CRAN. See line 105.

osorensen commented 11 months ago

👋 @adibender and @turgeonmaxime, could you please consider the author's response to you review issues, and update your checklists accordingly?

turgeonmaxime commented 11 months ago

@osorensen I'll update the checklist by the end of the week, hopefully over the next couple days

turgeonmaxime commented 11 months ago

@LingfengLuo0510 Thank you for addressing my previous comments. I especially appreciate the paragraph starting at line 116, it is really informative.

I had another look at the manuscript. I tried running the code line by line, but there are still some issues:

Finally, in the section "Availability", you wrote "Stable releases of the surtvep package will be made available via the Comprehensive R Archive Network." I think you can switch this to the present tense (i.e. "stable releases are available"), since it's now officially on CRAN!

turgeonmaxime commented 11 months ago

@osorensen I would appreciate your input for some of the points in the checklist:

Apart from that, and provided @LingfengLuo0510 addresses the remaining issues from my previous comment, I'm done with the review.

osorensen commented 11 months ago

Thanks for your review @turgeonmaxime. Here are my answers to your questions, which I hope can guide you on how to proceed further @LingfengLuo0510.

License file: It should by included. @LingfengLuo0510, please add a license file, and then also add the name of the license file to .Rbuildignore, so CRAN doesn't complain.

Testing: For R packages, there should be automated tests. @LingfengLuo0510, I suggest checking out the testthat package to get started. In addition to running on CRAN, the tests should also be part of an automated workflow in the GitHub repository, e.g., using GitHub Actions. You can read more on this in Wickham and Bryan's R packages book.

Contribution: This should be added.

osorensen commented 10 months ago

@LingfengLuo0510, I understand that addressing the review issues might take some time, but please keep us updated on the progress in this thread.

osorensen commented 10 months ago

@LingfengLuo0510, could you please respond here whether you're willing to complete the additional issues raised by the reviewers?

LingfengLuo0510 commented 10 months ago

@osorensen Thanks for asking. I am working on addressing the issues and will provide a detailed update within this week.

LingfengLuo0510 commented 9 months ago

@turgeonmaxime @adibender @osorensen Writing a comprehensive test takes longer than expected. We are still working on it.

We have added License file and Contribution Section. Thanks a lot!

LingfengLuo0510 commented 8 months ago

@turgeonmaxime @adibender @osorensen

In response to the review, we've incorporated testthat into our package and automated testing within our GitHub workflow. This ensures ongoing quality and facilitates future updates. We appreciate the feedback and are committed to continuous improvement. We will continue to update it in the future when adding new features. It is done for now for this paper review. Thanks a lot!

turgeonmaxime commented 8 months ago

@osorensen I've cloned the GH repo for the package and was able to run the tests successfully. With these last updates, I confirm that I've completed my review of this package/software note.

osorensen commented 8 months ago

Thanks @turgeonmaxime

osorensen commented 8 months ago

@adibender, could you please update your checklist based on the latest changes made by the authors?

adibender commented 8 months ago

@adibender, could you please update your checklist based on the latest changes made by the authors?

Hi, yes, will do sometime this week.

adibender commented 8 months ago

@adibender, could you please update your checklist based on the latest changes made by the authors?

Hi, yes, will do sometime this week.

Hi, I'm on it, but it'll take a little longer

osorensen commented 8 months ago

Thanks for the notice, @adibender

osorensen commented 8 months ago

@adibender, just a gentle reminder of this review. Thanks for all your efforts thus far.

osorensen commented 7 months ago

@adibender, could you please take a look at the revised package within the next week?

adibender commented 7 months ago

Hi @osorensen, sorry, the revision came at a bad time. Currently I'm on vacation and will be back on the 09th of April. Will do it until April 15th.

osorensen commented 7 months ago

Thanks for the response, @adibender. I'll set a reminder here in the issue.

osorensen commented 7 months ago

@editorialbot commands

editorialbot commented 7 months ago

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


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

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

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

# Remind an author, a reviewer or the editor to return to a review after a 
# certain period of time (supported units days and weeks)
@editorialbot remind @reviewer in 2 weeks

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

# Set a value for version
@editorialbot set v1.0.0 as version

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

# Set a value for repository
@editorialbot set https://github.com/organization/repo as repository

# Set a value for the archive DOI
@editorialbot set set 10.5281/zenodo.6861996 as archive

# Mention the EiCs for the correct track
@editorialbot ping track-eic

# Run checks and provide information on the repository and the paper file
@editorialbot check repository

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

# Generates the pdf paper
@editorialbot generate pdf

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Flag submission with questionable scope
@editorialbot query scope

# Get a link to the complete list of reviewers
@editorialbot list reviewers

# Creates a post-review checklist with editor and authors tasks
@editorialbot create post-review checklist

# Open the review issue
@editorialbot start review
osorensen commented 7 months ago

@editorialbot remind @adibender in 2 weeks

editorialbot commented 7 months ago

Reminder set for @adibender in 2 weeks