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

Submission: workloopR #326

Closed
vbaliga opened this issue Jul 22, 2019 · 33 comments
Assignees

Comments

@vbaliga
Copy link
Member

@vbaliga vbaliga commented Jul 22, 2019

Submitting Author: Vikram B. Baliga (@vbaliga)
Repository: https://github.com/vbaliga/workloopR
Version submitted: 1.0.0
Editor: @lmullen
Reviewer 1: @jromanowska
Reviewer 2: @eebrown
Archive: TBD
Version accepted: TBD


  • Paste the full DESCRIPTION file inside a code block below:
Package: workloopR
Type: Package
Title: Analysis of Work Loops and Other Data from Muscle Physiology Experiments
Version: 1.0.0
Authors@R: c(
    person("Vikram B.", "Baliga",
        email = "vbaliga87@gmail.com",
        role = c("cre", "aut"),
        comment = c(ORCID = "0000-0002-9367-8974")),
    person("Shreeram", "Senthivasan",
        email = "shreeramsenthi@gmail.com",
        role = "aut",
        comment = c(ORCID = "0000-0002-7118-9547")))
Author: Vikram B. Baliga [cre, aut] (<https://orcid.org/0000-0002-9367-8974>),
    Shreeram Senthivasan [aut] (<https://orcid.org/0000-0002-7118-9547>)
Maintainer: Vikram B. Baliga <vbaliga87@gmail.com>
Description: Functions for the import, transformation, and analysis of data from muscle physiology experiments. The work loop technique is used to evaluate the mechanical work and power output of muscle. Josephson (1985) <https://jeb.biologists.org/content/114/1/493> modernized the technique for application in comparative biomechanics. Although our initial motivation was to provide functions to analyze work loop experiment data, as we developed the package we incorporated the ability to analyze data from experiments that are often complementary to work loops. There are currently three supported experiment types: work loops, simple twitches, and tetanus trials. Data can be imported directly from .ddf files or via an object constructor function. Through either method, data can then be cleaned or transformed via methods typically used in studies of muscle physiology. Data can then be analyzed to determine the timing and magnitude of force development and relaxation (for isometric trials) or the magnitude of work, net power, and instantaneous power among other things (for work loops). Although we do not provide plotting functions, all resultant objects are designed to be friendly to visualization via either base-R plotting or 'tidyverse' functions.
URL: https://github.com/vbaliga/workloopR
BugReports:https://github.com/vbaliga/workloopR/issues
Imports:
    pracma (>= 2.0.7),
    signal (>= 0.7-6)
Suggests:
    testthat (>= 2.1.1),
    knitr,
    rmarkdown
License: GPL (>= 3) + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.1.9000
VignetteBuilder: knitr

Scope

  • Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):

    • data retrieval
    • data extraction
    • database access
    • data munging
    • data deposition
    • reproducibility
    • geospatial data
    • text analysis
  • Explain how and why the package falls under these categories (briefly, 1-2 sentences):
    This package offers the ability to import data from specialized file formats (including .ddf) that contain data from muscle physiology experiments. Data can then be transformed, re-organized, and analyzed according to methods typically used in studies of muscle physiology.

  • Who is the target audience and what are scientific applications of this package?
    Muscle physiologists or any other researchers interested in characterizing the properties of muscular tissue.

  • Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
    We know of no other packages in R that handle work loop and related data sets.

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Technical checks

Confirm each of the following by checking the box. This package:

Publication options

JOSS Options
  • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)
MEE Options
  • The package is novel and will be of interest to the broad readership of the journal.
  • The manuscript describing the package is no longer than 3000 words.
  • You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
  • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no guarantee that your manuscript will be within MEE scope.)
  • (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
  • (Please do not submit your package separately to Methods in Ecology and Evolution)

Code of conduct

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Aug 5, 2019

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

@vbaliga Thank you for your submission. I am currently looking for suitable reviewers. The review will begin when we have both in place.

In the meantime, here are a few things that you can work on.

  • Please run spelling::spell_check_package() in your package. I note a few genuine misspellings (and of course false positives).

Otherwise R CMD check looks very clean.

More TK.


Reviewers: @jromanowska @eebrown
Due date: 2019-10-21

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Aug 5, 2019

@lmullen Thank you for your feedback.

We used the spelling::spell_check_package() and took care of the misspellings throughout the package. The remaining items returned by that function are all proper nouns, specialized terms, and other false positives. We also fixed a few formatting issues in the package documentation. All changes are available now in the latest commit.

With respect to your editor checks, would you recommend that we deposit our package in a repository (e.g. figshare) after the review process? We are interested in submitting to JOSS and want to make sure we are taking the right steps. Please let me know if JOSS review is contingent on having the package in a repository.

Thanks!

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Aug 6, 2019

@vbaliga I generally think it is much easier to do the archival version of the package once the review is done. Doubtless the reviewers will make changes, and you can sync up the reviewed version, the CRAN version, and the figshare version if the package is accepted.

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Aug 6, 2019

@lmullen Sounds good - thanks!

@shreeramsenthi

This comment has been minimized.

Copy link

@shreeramsenthi shreeramsenthi commented Sep 17, 2019

Hi @lmullen, have there been any updates on finding potential reviewers? Thanks!

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Sep 17, 2019

@shreeramsenthi Yes, we are working on it. Please pardon the delay.

@shreeramsenthi

This comment has been minimized.

Copy link

@shreeramsenthi shreeramsenthi commented Sep 18, 2019

@lmullen No problem at all. Thanks for the update!

@sckott

This comment has been minimized.

Copy link
Member

@sckott sckott commented Sep 28, 2019

@shreeramsenthi been helping out trying to find reviewers, no luck yet. Do any of the workloopR authors have suggestions for reviewers?

@sckott

This comment has been minimized.

Copy link
Member

@sckott sckott commented Sep 30, 2019

reviewers are @eebrown and @jromanowska - due date 2019-10-21

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 1, 2019

@sckott and @lmullen thank you!

@jromanowska

This comment has been minimized.

Copy link

@jromanowska jromanowska commented Oct 8, 2019

Package Review

Please check off boxes as applicable, and elaborate in the comments below. Your review is not limited to these topics, as described in the reviewer guide

  • 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:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
    • The installation instruction could be expanded to point out that the default command to install from github will not create vignettes. While this may not be needed for all the users, I was a bit skeptic when I wanted to view the vignettes, as instructed in README.
  • Vignette(s) demonstrating major functionality that runs successfully locally
    • Very good vignettes. Two comments from me:
      • could the authors include links to the vignettes in README, where they refer to those?
      • please, order the vignettes differently, so as to at least include "Intro..." as the first one.
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
    • There were no performance claims.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines
    • wrong use of cat in data_import_functions.R (this should be message)
    • README does not contain a list of vignettes, nor it has links to the vignettes
    • Documentation: nicely created with pkgdown; I would create groups for listing the functions in Reference

Final approval (post-review)

  • [x ] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 16


Review Comments

It is a very nice package, with a clear aim, providing specific functions for analysis of data from muscle physiology experiments. Even though I am not within the field, I could easily understand what it does and how to use it. The README is very instructive, nicely written, with clear figures and a fantastic workflow scheme to help explain what's going on in the package. Very good!

In general, the authors made a very good job of structuring the code, commenting, creating documentation and testing. However, I have a few comments and suggestions:

  • is it necessary to have to install all the packages that are listed in vignettes? It takes a lot of time when creating those during the installation of the package.
  • in the vignette "Plotting data in workloopR", please delete the vnudge parameter in the last command, as it produces a warning.
  • why do the authors comment out some commands in the "examples" sections? Couldn't the "dontrun" command be used?
  • I would suggest splitting the code into more files --- right now, there are three .R files with all the functions, which can be a bit difficult to read and maintain, especially if someone else would like to contribute
  • please check that you use return command within each function to explicitly state what is the output of those
  • read_wl.ddf and read_twitch.ddf mix the naming conventions --- I would suggest rather read_wl_ddf and read_twitch_ddf
  • running spelling::spell_check_package() on the package showed that perhaps the authors should change "filetype", "filepath" to "file type" and "file path", respectively
  • running goodpractice::gp() showed that there three things the authors should consider correcting:
    • too long lines in some of the functions
    • using both, "<-" and "=" is discouraged
    • change sapply to vapply, change 1:length() to seq_along

Check also the output generated with the pkgreviewr for details on testing and automated checks.

@eebrown

This comment has been minimized.

Copy link
Member

@eebrown eebrown commented Oct 8, 2019

Package Review

  • 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).

There is no conflict, but I note a caveat that this field is outside of my domain and I am doing this review from a technical perspective, as I understand it was hard to find reviewers within the domain with R knowledge.

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README

This package is designed to analyze data from a specific set of muscle physiology experiments. More specifically, it analyzes the output from DDF files generated from a 3rd party proprietary device. The package’s features are explained in the README as well as the DESCRIPTION.

  • Installation instructions: for the development version of package and any non-standard dependencies in README

Yes, installation was straightforward.

  • Vignette(s) demonstrating major functionality that runs successfully locally

The README provides a good overview of the features and available functions. There are several useful vignettes for the package. Given the work that has gone into creating these, it would be nice to have hyperlinks from the README page to access the rendered pages from the browser (the pkgdown website).

  • Function Documentation: for all exported functions in R help

Functions are all documented as expected. Functions do not all have explicit return statements which is allowed but not ideal and makes code review harder in my opinion. I am surprised I don't see an @ropensci policy on this, so I'll leave it up to the authors/editor.

  • Examples for all exported functions in R Help that run successfully locally

  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Please add contributor guidelines (esp. as you request particular types of contributors, e.g. to add support for non DDF file formats). e.g. https://devguide.ropensci.org/collaboration.html

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.

The package has full test coverage, including the expected results. However, it does not explain how the authors obtained the expected results for the tests.

It may be helpful to briefly explain the approach that was used for these analyses prior to this package (e.g. in the use case, what this is replacing). Further, it could be noted that (whether) the package produces the same results as this other method (was it some other software, was it manually). That would help prove the package functions as it claims, particularly since I don't have access to data to use to test the package for accuracy.

Are the default parameters (e.g. those in select_cycles) safe? I.e. would it be catastrophic if a user overlooked those arguments and didn't realize the defaults were in place? If so, it may be better not to have default arguments, and add tests to ensure the given arguments are possible values (rather than just numeric or not).

  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Passes the basic devtools::check().

Please add the rOpenSci review badge to the README (https://devguide.ropensci.org/building.html#readme)

In your DESCRIPTION file, the Author and Maintainer fields should not be included as they are redundant (they are generated from the Authors@R field).

Are there improvements that could be made to the code style?

A minor point is that the authors don't use the recommended spacing, e.g. they write rising<-rising/100 instead of rising <- rising / 100. For these kinds of minor changes, consider using the styler package (https://github.com/r-lib/styler), as recommended (https://devguide.ropensci.org/building.html#code-style).

Running goodpractice::gp() provides the following suggestions which can be addressed:

It is good practice to

✖ use '<-' for assignment instead of '='. '<-' is the
standard, and R users and developers are used it and it is easier
to read your code for them if you use '<-'.

R/data_analysis_functions.R:61:4
R/data_analysis_functions.R:63:11
R/data_analysis_functions.R:476:13
R/data_import_functions.R:275:11

✖ avoid long code lines, it is bad for readability. Also,
many people prefer editor windows that are about 80 characters
wide. Try make your lines shorter than 80 characters

R/data_analysis_functions.R:132:1
R/data_analysis_functions.R:224:1
R/data_analysis_functions.R:228:1
R/data_analysis_functions.R:249:1
R/data_analysis_functions.R:250:1
... and 98 more lines

✖ avoid sapply(), it is not type safe. It might return a
vector, or a list, depending on the input data. Consider using
vapply() instead.

R/data_analysis_functions.R:506:15
R/data_analysis_functions.R:507:17
R/data_analysis_functions.R:516:7
R/data_analysis_functions.R:518:7
R/data_import_functions.R:582:26
... and 24 more lines

✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...),
1:NROW(...) and 1:NCOL(...) expressions. They are error prone and
result 1:0 if the expression on the right hand side is zero. Use
seq_len() or seq_along() instead.

R/data_import_functions.R:275:13
R/data_import_functions.R:605:32

Please also note the typo on line 314 of data_import_functions.R.

Is there code duplication in the package that should be reduced?

You may wish to replace the internal function trapezoidal_integration() with pracma::trapz(), as you already import pracma, which is a more established package, and it would give you less to test/maintain.

Are there user interface improvements that could be made?

Linking to the documentation website from the repository/README could be helpful.

Are there performance improvements that could be made?

Appears fine to me.

Instead of just warning people that the units are assumed to be seconds, millimeters, and millinewtons, would it also be helpful to test that the data are in a reasonable range expected for those units? I assume there is a reasonable range for all muscle physiology experiments.

Is the documentation (installation instructions/vignettes/examples/demos) clear and sufficient?

Yes.

Does it use the principle of multiple points of entry i.e. takes into account the fact that any piece of documentation may be the first encounter the user has with the package and/or the tool/data it wraps?

Yes it appears sufficient and detailed, although it would be best to have this assessed by the target audience.

Were functions and arguments named to work together to form a common, logical programming API that is easy to read, and autocomplete?

Function names are logical and in families.

If you have your own relevant data/problem, work through it with the package. You may find rough edges and use-cases the author didn’t think about.

Are there any DDF files from other sources (e.g. online repositories, published articles) that could be used to test?

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 10

  • Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

My comments/suggestions are interspersed above. Overall I think the premise and design is excellent. On the face of it, this package seems very useful. My main concern as a reviewer from outside of the domain of muscle physiology, is that I cannot test it for accuracy. Please let me know if there is any way we can demonstrate this package provides a similar result to an existing analysis work flow, or commercial software, or previously done/published manual work, etc.

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 17, 2019

@jromanowska and @eebrown Thank you both very much for your reviews. @shreeramsenthi and I are now going to work on the issues you both raised. We'll likely do this in several stages and spaced across a few commits. We'll notify you via this thread when we think we have responded to all your feedback and if we have any follow-up questions. Thanks again!

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 23, 2019

Hi @jromanowska and @eebrown,

Thank you both very much for your thorough reviews of workloopR. @shreeramsenthi and I feel that we have tackled most of the items you brought up. Please feel free to let us know if we can make any further improvements.

Also, as noted below, we took the liberty to list both of you as reviewers in our DESCRIPTION file. Please let us know if you'd prefer to not be listed or if you'd like any details changed.

Best regards,
Vikram
🐢

Response to Package Review by @jromanowska

Documentation

  • Installation instructions: The installation instruction could be expanded to point out that the default command to install from github will not create vignettes. While this may not be needed for all the users, I was a bit skeptic when I wanted to view the vignettes, as instructed in README.

Response We have edited the README with a note about building vignettes along with code in ropensci/workloopR@002ec08

  • Vignette(s): Very good vignettes. Two comments from me:
    • could the authors include links to the vignettes in README, where they refer to those?
    • please, order the vignettes differently, so as to at least include "Intro..." as the first one.

Response We have added links to the vignettes within the README and reordered the vignette list on the pkgdown site in ropensci/workloopR@77e563c

Functionality

  • Performance: There were no performance claims

Response Indeed, we make no performance claims as speed of execution does not seem to be a concern right now. Is this OK?

  • Packaging guidelines:
    • wrong use of cat() in data_import_functions.R (this should be message)

Response Could we please have clarification? cat() is used in three situations in data_import_functions.R:
1. print methods for workloopR-specific S3 objects (muscle_stim, tetanus, etc.)
2. summary methods for workloopR-specific S3 objects
3. print_muscle_stim_header(), a non-exported function that is only ever called by the various print methods

We see that points ii and iii do not fall strictly under the rOpenSci guidelines for using cat(), but they seem to fit a similar use case of displaying info from an object, rather than signaling condition like most message() calls we've come across. Would this use be OK?

  • Packaging guidelines:
    • README does not contain a list of vignettes, nor it has links to the vignettes

Response As noted above, we added links to the vignettes within the README in ropensci/workloopR@77e563c

  • Packaging guidelines:
    • Documentation: nicely created with pkgdown; I would create groups for listing the functions in Reference

Response This is a great suggestion and we were happy to comply: ropensci/workloopR@8f0de29

Review Comments

  • is it necessary to have to install all the packages that are listed in vignettes? It takes a lot of time when creating those during the installation of the package.

Response Although the package does not depend on any tidyverse packages, the functions were written to fit into a tidyverse-centric workflow with minimal friction. Accordingly, we feel it is important to demonstrate this in our vignettes, which requires using functions from a variety of tidyverse packages. All other dependencies were removed ropensci/workloopR@b2abd81

  • in the vignette "Plotting data in workloopR", please delete the vnudge parameter in the last command, as it produces a warning.

Response The vnudge parameter has been deleted: ropensci/workloopR@77e563c

  • why do the authors comment out some commands in the "examples" sections? Couldn't the "dontrun" command be used?

Response Thank you for this suggestion. We swapped all such cases to use dontrun() in: ropensci/workloopR@a26cc7a

  • I would suggest splitting the code into more files --- right now, there are three .R files with all the functions, which can be a bit difficult to read and maintain, especially if someone else would like to contribute

Response We agree and split the code into three additional files: ropensci/workloopR@07cfdef

  • please check that you use return command within each function to explicitly state what is the output of those

Response We ensured return() was used in: ropensci/workloopR@4943ecc

  • read_wl.ddf and read_twitch.ddf mix the naming conventions --- I would suggest rather read_wl_ddf and read_twitch_ddf

Response We agree and made the change here: ropensci/workloopR@77e563c

  • running spelling::spell_check_package() on the package showed that perhaps the authors should change "filetype", "filepath" to "file type" and "file path", respectively

Response Thank you for catching this. Changes were made here: ropensci/workloopR@4d7b48c

  • running goodpractice::gp() showed that there three things the authors should consider correcting:
    • too long lines in some of the functions

Response We limited lines to 80 characters in: ropensci/workloopR@4d7b48c

  • using both, "<-" and "=" is discouraged

Response We agree and edited this in a few commits: ropensci/workloopR@56ee2c7, and ropensci/workloopR@d654fec

  • change sapply to vapply, change 1:length() to seq_along

Response We agree and also edited this across a few commits: ropensci/workloopR@a1abf37, ropensci/workloopR@48b453b, and ropensci/workloopR@5c74837

Note We both thank you for your helpful review. We would like to acknowledge your help and took the liberty of adding you as a reviewer in our DESCRIPTION file. Please let us know if you would not like to be acknowledged in this way or if you would like to censor or change any of your contact information.


Response to Package Review by @eebrown

Documentation

  • Vignette(s): The README provides a good overview of the features and available functions. There are several useful vignettes for the package. Given the work that has gone into creating these, it would be nice to have hyperlinks from the README page to access the rendered pages from the browser (the pkgdown website).

Response Thank you for your suggestion. We added links to the vignettes in the README in: ropensci/workloopR@77e563c

  • Function Documentation: Functions are all documented as expected. Functions do not all have explicit return statements which is allowed but not ideal and makes code review harder in my opinion. I am surprised I don't see an @ropensci policy on this, so I'll leave it up to the authors/editor.

Response We agree and added return() statements to all named functions in: ropensci/workloopR@4943ecc

  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogeneated via Authors@R).
    Please add contributor guidelines (esp. as you request particular types of contributors, e.g. to add support for non DDF file formats). e.g. https://devguide.ropensci.org/collaboration.html

Response Thank you for catching this. We were happy to add in contribution guidelines via rodev::use_ro_github() in: ropensci/workloopR@5cd03bd

Functionality

  • Functionality: Any functional claims of the software been confirmed.

The package has full test coverage, including the expected results. However, it does not explain how the authors obtained the expected results for the tests.

It may be helpful to briefly explain the approach that was used for these analyses prior to this package (e.g. in the use case, what this is replacing). Further, it could be noted that (whether) the package produces the same results as this other method (was it some other software, was it manually). That would help prove the package functions as it claims, particularly since I don't have access to data to use to test the package for accuracy.

Response We agree with the points you make. Validity of the output is indeed something that should require due diligence. The approach that is typically used for these analyses is to use proprietary software such as Dynamic Muscle Analysis (DMA) by Aurora Software (see here). The method for analysis by this and any other (valid) software follows what we show in Figure 1 of our JOSS paper: mechanical work is the product of force and distance and the area within a work loop is the net work produced by the muscle. Our own method of verification was to check the force, length, timing, net work reported by DMA against what our functions produce. We have done this for each of the example .ddf files included in workloopR. All values of force, timing, and net work are nearly identical to what DMA produces. At present, we are not sure the best way to show this to you and ropensci's editors. It is hard to generate an output file from DMA; instead values of e.g. net work are simply reported on-screen. Would supplying a variety of screenshots be sufficient? Happy to follow up as necessary

Are the default parameters (e.g. those in select_cycles) safe? I.e. would it be catastrophic if a user overlooked those arguments and didn't realize the defaults were in place? If so, it may be better not to have default arguments, and add tests to ensure the given arguments are possible values (rather than just numeric or not).

Response The default parameters in our functions conform to typical protocols in muscle physiology experiments, e.g. it is common to use cycles 3:5 in work loop studies. In that sense, our defaults should generally be safe. We also encourage users to think carefully about how their data are being analyzed and issue a variety of Warnings within many of the functions.

Response The badge has been added: ropensci/workloopR@a77eb6d

  • Packaging guidelines In your DESCRIPTION file, the Author and Maintainer fields should not be included as they are redundant (they are generated from the Authors@R field).

Response The redundant Author and Maintainer fields have been removed in: ropensci/workloopR@b6184be

Are there improvements that could be made to the code style? A minor point is that the authors don't use the recommended spacing, e.g. they write rising<-rising/100 instead of rising <- rising / 100. For these kinds of minor changes, consider using the styler package (https://github.com/r-lib/styler), as recommended (https://devguide.ropensci.org/building.html#code-style).

Response Thanks for the suggestion. We used the styler package as recommended: ropensci/workloopR@56ee2c7 and ropensci/workloopR@d654fec

Running goodpractice::gp() provides the following suggestions which can be addressed:

Response Thanks for catching these. These have been addressed across a couple commits, most notably:
- Removed sapply in: ropensci/workloopR@48b453b and ropensci/workloopR@5c74837
- Removed 1:length(...), etc. in: ropensci/workloopR@a1abf37
- Broke up long lines in: ropensci/workloopR@f8c85ba
- Used '<-' consistently in: ropensci/workloopR@56ee2c7

Please also note the typo on line 314 of data_import_functions.R.

Response Thanks -- this has been fixed in: ropensci/workloopR@4ea5ea7

Is there code duplication in the package that should be reduced? You may wish to replace the internal function trapezoidal_integration() with pracma::trapz(), as you already import pracma, which is a more established package, and it would give you less to test/maintain.

Response Although we see your point, we'd prefer to keep our function trapezoidal_integration(). We think it is a function that should be straightforward to test/maintain and keeping it in the package makes for one less thing to worry about if the authors of pracma choose to edit trapz().

Are there user interface improvements that could be made? Linking to the documentation website from the repository/README could be helpful.

Response Thanks for the suggestion; we added a link to the pkgdown site within the README in: ropensci/workloopR@77e563c

Instead of just warning people that the units are assumed to be seconds, millimeters, and millinewtons, would it also be helpful to test that the data are in a reasonable range expected for those units? I assume there is a reasonable range for all muscle physiology experiments.

Response Although this would be nice to have, it is actually hard to predict what a reasonable range might be for a few of these units. Studies of muscle physiology range from examining minuscule flies to fish to large mammals and we hope this package will be useful to all such cases. Accordingly, it is hard to predict the scale of each variable as it will be strongly driven by organismal or muscular size.

If you have your own relevant data/problem, work through it with the package. You may find rough edges and use-cases the author didn’t think about. Are there any DDF files from other sources (e.g. online repositories, published articles) that could be used to test?

Response A big source of frustration for us is the lack of open data in muscle physiology experiments in addition to the heavy reliance on proprietary software (hence, workloopR). The only other .ddf files that we know of in a public repository can be found in this Figshare repository. Please note that the study to which this repository corresponds is also from our laboratory (Altshuler Lab at UBC). So these .ddf files may be used to test our package, but please note there may be a potential conflict of interest (not exactly sure how this works).

Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Response Absolutely. We are very happy to do this and took the liberty to copy information about you that was available in the chlorpromazineR package DESCRIPTION file. Please let us know if you would like to change any of the information. Commit here: ropensci/workloopR@f0bad97

Review Comments

My comments/suggestions are interspersed above. Overall I think the premise and design is excellent. On the face of it, this package seems very useful. My main concern as a reviewer from outside of the domain of muscle physiology, is that I cannot test it for accuracy. Please let me know if there is any way we can demonstrate this package provides a similar result to an existing analysis work flow, or commercial software, or previously done/published manual work, etc.

Response Thanks again for all your feedback -- we found your review to be very helpful. As noted above, we're happy to follow up and show how our package produces similar results to those achieved in Aurora Software's Dynamic Muscle Analysis software (proprietary; see here). If screenshots are sufficient, we can post them to this issue thread.

@jromanowska

This comment has been minimized.

Copy link

@jromanowska jromanowska commented Oct 23, 2019

Great! Thank you for the very good response and description of the changes. A propos this "cat" usage - I can't find now any place that it's used wrongly. Maybe I've just misjudged, so you can forget about this.
I am very satisfied with the changes the authors made. Great work!

@eebrown

This comment has been minimized.

Copy link
Member

@eebrown eebrown commented Oct 23, 2019

Hi @vbaliga,
Thank you for addressing my feedback. I think your suggestion to show a screenshot of the commercial software output so that it could be compared side by side with your package's output would be a great asset this review as it would help me see how your results compare to the current standard. I understand that there is not much open data available from other users to test at this time (and hopefully your package inspires more open research in this field!).
Otherwise, great work.
Thanks,
Eric

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 24, 2019

Hi @eebrown,

Please see this document, which we prepared to show the results of workloopR against those of commercial software. We showcase results using two types of experimental files. Let us know if you think what we've supplied is sufficient or if you have any follow-up questions.

Thanks again!
Vikram

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 24, 2019

Hi @jromanowska,

Great - and thanks again for your helpful review!

Cheers,
Vikram

@eebrown

This comment has been minimized.

Copy link
Member

@eebrown eebrown commented Oct 25, 2019

Dear @vbaliga,

Thank you very much for your thorough comparison of the package's output to an existing analysis suite (https://github.com/ropensci/software-review/files/3769683/workloopR_vs_DMA.pdf). You clearly showed that your package produces the expected results with your supplied data files, and also illustrated the major benefits of your package in terms of replicability and open science. I appreciate the work you did to respond to this review and also hope that this comparison will be helpful to others considering using your package.

I have no further concerns and recommend this package for inclusion in rOpenSci. Congratulations and all the best.

Eric

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 25, 2019

Hi @eebrown,
Great! Thanks again for all your feedback. We appreciate your recommendation

@lmullen We believe that both reviewers are satisfied with the changes we made to workloopR. Please let us know if you have any feedback about the changes we made to the package, any comments on further changes that you'd like to see, and/or instructions for next steps

Thanks again!
Vikram

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Oct 26, 2019

@jromanowska @eebrown Thank you both for the thorough reviews and for going back and forth with the authors. Could you please both check off the relevant boxes, especially the ones about finally approving the package?

@vbaliga Thanks for the very clear responses to the reviewers work. Once they've checked off all the boxes we can work on next steps for moving this into rOpenSci.

@jromanowska

This comment has been minimized.

Copy link

@jromanowska jromanowska commented Oct 26, 2019

Here is the final acceptance, after the authors replied to all the comments I had. Great work!


Package Review

Please check off boxes as applicable, and elaborate in the comments below. Your review is not limited to these topics, as described in the reviewer guide

  • 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:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
    • There were no performance claims.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 16

@eebrown

This comment has been minimized.

Copy link
Member

@eebrown eebrown commented Oct 27, 2019

@lmullen Thanks, I've gone back and checked all the boxes.

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Oct 30, 2019

Congratulations, @vbaliga, we will now consider this package to be accepted. @eebrown and @jangorecki, thanks again for your thorough and helpful reviews.

@vbaliga There are some tasks that we will ask you to undertake in order to get the repository officially into rOpenSCi.

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I will shortly invited you to a team that should allow you to do so. Please let me know when this is done so that I can make you an admin once you do.
  • Add the rOpenSci footer to the bottom of your README
    " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
  • Fix all links to the GitHub repo to point to the repo under the ropensci organization.
  • If you already had a pkgdown website, fix its URL to point to https://docs.ropensci.org/package_name and deactivate the automatic deployment you might have set up, since it will not be built centrally like for all rOpenSci packages, see http://devdevguide.netlify.com/#docsropensci. In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
  • Add a mention of the review in DESCRIPTION via rodev::add_ro_desc().
  • Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname).
  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.
  • Activate Zenodo watching the repo
  • Tag and create a release so as to create a Zenodo version and DOI
  • Submit to JOSS at https://joss.theoj.org/papers/new, using the rOpenSci GitHub repo URL. When a JOSS "PRE REVIEW" issue is generated for your paper, add the comment: This package has been reviewed by rOpenSci: https://LINK.TO/THE/REVIEW/ISSUE, @ropensci/editors

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 30, 2019

Hi @lmullen

Thank you very much!

I just accepted your invitation to join the rOpenSci organization and have now transferred ownership to ropensci -- please make me an admin whenever you get the chance.

Prior to transferring ownership of workloopR to ropensci, I took care of some of the items below. I'll follow up with the remaining items shortly.

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I will shortly invited you to a team that should allow you to do so. Please let me know when this is done so that I can make you an admin once you do.
  • Add the rOpenSci footer to the bottom of your README
    " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
  • Fix all links to the GitHub repo to point to the repo under the ropensci organization.
  • If you already had a pkgdown website, fix its URL to point to https://docs.ropensci.org/package_name and deactivate the automatic deployment you might have set up, since it will not be built centrally like for all rOpenSci packages, see http://devdevguide.netlify.com/#docsropensci. In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
  • Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname).
  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

These items remain (momentarily). I'll confirm once these have been taken care of:

  • Activate Zenodo watching the repo
  • Tag and create a release so as to create a Zenodo version and DOI
  • Submit to JOSS at https://joss.theoj.org/papers/new, using the rOpenSci GitHub repo URL. When a JOSS "PRE REVIEW" issue is generated for your paper, add the comment: This package has been reviewed by rOpenSci: https://LINK.TO/THE/REVIEW/ISSUE, @ropensci/editors

Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

@stefaniebutland I am interested in contributing a blog post and would be happy to follow up about details

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Oct 30, 2019

@vbaliga You should have admin rights on the repo now. Please let me know if you have any problems with that.

@lmullen lmullen closed this Oct 30, 2019
@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 30, 2019

Thanks again @lmullen . I have now taken care of the remaining (Zenodo/JOSS) items.

  • Activate Zenodo watching the repo

  • Tag and create a release so as to create a Zenodo version and DOI

EDIT: Because we have made a new version as part of the JOSS submission, I am editing this message to now list the DOI that is linked to all workloopR versions:
The DOI is: 10.5281/zenodo.3523384
DOI

Please note that the JOSS PRE-REVIEW issue hasn't yet been generated. I'll be sure to follow up with posting the specified message when that happens.

Thanks again for all your help!

@stefaniebutland

This comment has been minimized.

Copy link

@stefaniebutland stefaniebutland commented Oct 30, 2019

Hi @vbaliga. Congratulations on your package acceptance!

I think a tech note rather than a full length blog post on workloopR would be most appropriate since it has a very specific target audience. Instructions are here https://github.com/ropensci/roweb2#contributing-a-blog-post. Technically same as a blog post, but in YAML, set categories: technotes instead of categories: blog.

Your tech note could say what the package does and what unmet need it fills and provide a "cool" example use, without duplicating the vignette.

Please suggest a date on which you would like to submit a draft.

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 31, 2019

Hi @stefaniebutland - thank you for reaching out. I'd be happy to write a tech note for the blog. Thank you for your advice on how it should flow; I'm happy to follow your recommendations.

Would a draft submission on or before Nov 9th be ok?

@stefaniebutland

This comment has been minimized.

Copy link

@stefaniebutland stefaniebutland commented Oct 31, 2019

That sounds good. Please use date: 2019-11-14 in YAML as the tentative publication date. Once submitted, I'll review 🙂

If there is a higher profile blog post published in the same week, I might propose a change to your date so your tech note isn't lost in the noise

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Oct 31, 2019

@stefaniebutland Sounds good - thanks!

@vbaliga

This comment has been minimized.

Copy link
Member Author

@vbaliga vbaliga commented Nov 1, 2019

Hi @lmullen, @jromanowska, @eebrown ,

I am please to share that our paper has now been published in JOSS: https://doi.org/10.21105/joss.01856

Thanks again for all your help in reviewing workloopR and for suggesting improvements. @shreeramsenthi and I are both very happy with how everything turned out (but of course, we're happy to take further suggestions on features...etc).

Best regards,
Vikram
🐢

@lmullen

This comment has been minimized.

Copy link
Member

@lmullen lmullen commented Nov 2, 2019

🎊 Congratulations, @vbaliga. Glad to see this published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.