Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
editorialbot opened this issue Jul 24, 2023 · 129 comments
Closed

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

editorialbot opened this issue Jul 24, 2023 · 129 comments
Assignees
Labels
accepted C++ HTML published Papers published in JOSS R recommend-accept Papers recommended for acceptance in JOSS. review TeX Track: 5 (DSAIS) Data Science, Artificial Intelligence, and Machine Learning

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Jul 24, 2023

Submitting author: @LingfengLuo0510 (Lingfeng Luo)
Repository: https://github.com/UM-KevinHe/surtvep
Branch with paper.md (empty if default branch): JOSS
Version: v1.0.0
Editor: @osorensen
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 editorialbot added HTML R review TeX Track: 5 (DSAIS) Data Science, Artificial Intelligence, and Machine Learning waitlisted Submissions in the JOSS backlog due to reduced service mode. labels Jul 24, 2023
@editorialbot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Checking the BibTeX entries failed with the following error:

Failed to parse BibTeX on value "title" (NAME) [#<BibTeX::Bibliography data=[8]>, "@", #<BibTeX::Entry >, "%"]

@editorialbot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Wordcount for paper.md is 1759

@editorialbot
Copy link
Collaborator Author

Failed to discover a valid open source license

@editorialbot
Copy link
Collaborator Author

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@turgeonmaxime
Copy link

turgeonmaxime commented Jul 24, 2023

Review checklist for @turgeonmaxime

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/UM-KevinHe/surtvep?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@LingfengLuo0510) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • 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.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • 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.

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • 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?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

@adibender
Copy link

adibender commented Jul 25, 2023

Review checklist for @adibender

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/UM-KevinHe/surtvep?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@LingfengLuo0510) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • 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.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • 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.

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • 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?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

@adibender
Copy link

@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
Copy link
Member

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

@turgeonmaxime
Copy link

turgeonmaxime commented Aug 10, 2023

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

  • 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.
  • In Figure 2, the confidence bands don't cover the same range as the curve estimate. Why is that?
  • 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.
  • For the section "Data Example", would it be possible to share your code?
  • 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?

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
Copy link

turgeonmaxime commented Aug 10, 2023

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/DESCchecking for file/private/var/folders/hb/r5l2r5b12gg5y2zxyg4l7nh80000gn/T/Rtmp42cwos/remotes1030ea884ab0/UM-KevinHe-surtvep-31f9fd4/DESCRIPTION’
─  preparingsurtvep:checking DESCRIPTION meta-information ...cleaning srcchecking for LF line-endings in source and make files and shell scriptschecking for empty or unneeded directoriesbuildingsurtvep_1.0.0.tar.gz* installing *source* packagesurtvep...
** 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.sdkclang++ -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 packagesurtvep* removing/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/surtvep* restoring previous/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/surtvepWarning message:
In i.p(...) :
  installation of package/var/folders/hb/r5l2r5b12gg5y2zxyg4l7nh80000gn/T//Rtmp42cwos/file1030e79df6e36/surtvep_1.0.0.tar.gzhad 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
Copy link

turgeonmaxime commented Aug 10, 2023

@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
Copy link
Member

Thanks a lot for you comments @turgeonmaxime!

@editorialbot
Copy link
Collaborator Author

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@osorensen
Copy link
Member

@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
Copy link
Member

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

@LingfengLuo0510
Copy link

@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
Copy link

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

  • LICENSE file
  • automated tests (e.g. using testthat package)
  • Community guidelines (how to contribute)

@osorensen
Copy link
Member

Thanks @adibender.

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

@LingfengLuo0510
Copy link

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.

@crvernon
Copy link

👋 @LingfengLuo0510 - thanks for all of your hard work on this. Your paper is very clean and that is much appreciated! I just need you to address a few final things before we accept this for publication:

  • Please edit the metadata in your Zenodo archive to make the license match what you use in your repository. Currently your license in the archive is specified as Creative Commons Attribution 4.0 International.
  • Please match the author order in your Zenodo archive to what is in your paper.

Let me know when these are done and I will move to accept.

@LingfengLuo0510
Copy link

👋 @LingfengLuo0510 - thanks for all of your hard work on this. Your paper is very clean and that is much appreciated! I just need you to address a few final things before we accept this for publication:

  • Please edit the metadata in your Zenodo archive to make the license match what you use in your repository. Currently your license in the archive is specified as Creative Commons Attribution 4.0 International.
  • Please match the author order in your Zenodo archive to what is in your paper.

Let me know when these are done and I will move to accept.

@crvernon

Thanks a lot!
(1) I have updated the license. It matches what I use in my repository.
(2) I have adjusted the auther order. It matches the author order now.

Thanks again!

@crvernon
Copy link

@editorialbot accept

@editorialbot
Copy link
Collaborator Author

Doing it live! Attempting automated processing of paper acceptance...

@editorialbot
Copy link
Collaborator Author

Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository.

If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file.

You can copy the contents for your CITATION.cff file here:

CITATION.cff

cff-version: "1.2.0"
authors:
- family-names: Luo
  given-names: Lingfeng
- family-names: Wu
  given-names: Wenbo
- family-names: Taylor
  given-names: Jeremy
- family-names: Kang
  given-names: Jian
- family-names: Kleinsasser
  given-names: Michael
- family-names: He
  given-names: Kevin
contact:
- family-names: He
  given-names: Kevin
doi: 10.5281/zenodo.12575049
message: If you use this software, please cite our article in the
  Journal of Open Source Software.
preferred-citation:
  authors:
  - family-names: Luo
    given-names: Lingfeng
  - family-names: Wu
    given-names: Wenbo
  - family-names: Taylor
    given-names: Jeremy
  - family-names: Kang
    given-names: Jian
  - family-names: Kleinsasser
    given-names: Michael
  - family-names: He
    given-names: Kevin
  date-published: 2024-06-28
  doi: 10.21105/joss.05688
  issn: 2475-9066
  issue: 98
  journal: Journal of Open Source Software
  publisher:
    name: Open Journals
  start: 5688
  title: "surtvep: An R package for estimating time-varying effects"
  type: article
  url: "https://joss.theoj.org/papers/10.21105/joss.05688"
  volume: 9
title: "surtvep: An R package for estimating time-varying effects"

If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation.

Find more information on .cff files here and here.

@editorialbot
Copy link
Collaborator Author

🐘🐘🐘 👉 Toot for this paper 👈 🐘🐘🐘

@editorialbot
Copy link
Collaborator Author

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.05688 joss-papers#5550
  2. Wait five minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.05688
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? Notify your editorial technical team...

@editorialbot editorialbot added accepted published Papers published in JOSS labels Jun 28, 2024
@crvernon
Copy link

🥳 Congratulations on your new publication @LingfengLuo0510! Many thanks to @osorensen for editing and @adibender and @turgeonmaxime for your time, hard work, and expertise!! JOSS wouldn't be able to function nor succeed without your efforts.

Please consider becoming a reviewer for JOSS if you are not already: https://reviewers.joss.theoj.org/join

@editorialbot
Copy link
Collaborator Author

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.05688/status.svg)](https://doi.org/10.21105/joss.05688)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.05688">
  <img src="https://joss.theoj.org/papers/10.21105/joss.05688/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.05688/status.svg
   :target: https://doi.org/10.21105/joss.05688

This is how it will look in your documentation:

DOI

We need your help!

The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@LingfengLuo0510
Copy link

@crvernon I feel so sorry that may I ask if I can update the middle name of one of the authors? So sorry about this.

@crvernon
Copy link

Yes please do so. I am getting ready to board a flight but will reaccept this as soon as I can. Just let me know once you have made the change.

@LingfengLuo0510
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

@LingfengLuo0510
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

@LingfengLuo0510
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

@LingfengLuo0510
Copy link

Yes please do so. I am getting ready to board a flight but will reaccept this as soon as I can. Just let me know once you have made the change.

@crvernon Than
Thanks a lot! I just updated the paper.md file
(1) Jeremy Taylor -> Jeremy M.G. Taylor
(2) Michael Kleinsasser -> Michael J. Kleinsasser

However, I'm encountering errors when generating the PDF, even after reverting to the previously accepted version. Could this be related to the paper's accepted status?

@crvernon crvernon reopened this Jun 28, 2024
@crvernon
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

@crvernon
Copy link

@editorialbot reaccept

@editorialbot
Copy link
Collaborator Author

Rebuilding paper!

@editorialbot
Copy link
Collaborator Author

🌈 Paper updated!

New PDF and metadata files 👉 openjournals/joss-papers#5551

@crvernon
Copy link

@LingfengLuo0510 - the paper should now reflect your changes.

@editorialbot
Copy link
Collaborator Author

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.05688/status.svg)](https://doi.org/10.21105/joss.05688)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.05688">
  <img src="https://joss.theoj.org/papers/10.21105/joss.05688/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.05688/status.svg
   :target: https://doi.org/10.21105/joss.05688

This is how it will look in your documentation:

DOI

We need your help!

The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted C++ HTML published Papers published in JOSS R recommend-accept Papers recommended for acceptance in JOSS. review TeX Track: 5 (DSAIS) Data Science, Artificial Intelligence, and Machine Learning
Projects
None yet
Development

No branches or pull requests

6 participants