[REVIEW]: activAnalyzer: An R Shiny app to analyse ActiGraph accelerometer data and to implement the use of the PROactive Physical Activity in COPD instruments

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@pydemull@samhforbes<!--end-editor-- Reviewers: @elimillera, @angerhang Archive: 10.5281/zenodo.7384191

Status

Status badge code:

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

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

@elimillera & @spitschan & @angerhang, 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 @samhforbes 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 @angerhang

📝 Checklist for @elimillera

editorialbot commented 2 years 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 2 years ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.09 s (1125.1 files/s, 244759.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               68           1803           2113          11621
XML                              1              0            129           1589
Markdown                         6            207              0            825
TeX                              4             55              0            771
Rmd                             10            298            683            657
CSS                              2             62             16            274
YAML                             7             35              8            173
-------------------------------------------------------------------------------
SUM:                            98           2460           2949          15910
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 2 years ago

Wordcount for paper.md is 1135

editorialbot commented 2 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.pcad.2014.10.005 is OK
- 10.1016/j.rmed.2021.106353 is OK
- 10.1111/sms.14085 is OK
- 10.1249/MSS.0000000000000468 is OK
- 10.1136/bmjresp-2018-000350 is OK
- 10.1249/MSS.0b013e318258cb36 is OK
- 10.1249/MSS.0b013e3181fc7162 is OK
- 10.15326/jcopdf.2021.0213 is OK
- 10.1183/09031936.00004814 is OK
- 10.1136/bjsports-2019-101765 is OK
- 10.1136/thoraxjnl-2020-214554 is OK
- 10.1093/gerona/53A.4.M275 is OK
- 10.1183/09031936.00183014 is OK
- 10.1371/journal.pone.0077036 is OK
- 10.1249/MSS.0b013e3182399dcc is OK
- 10.1249/MSS.0000000000001073 is OK
- 10.1016/j.arbres.2020.08.001 is OK
- 10.1007/s40279-017-0716-0 is OK
- 10.1136/bmjresp-2020-000737 is OK
- 10.1016/S2213-2600(20)30105-3 is OK
- 10.1183/09031936.00023214 is OK
- 10.2147/COPD.S214410 is OK
- 10.32614/RJ-2014-024 is OK

MISSING DOIs

- None

INVALID DOIs

- None

editorialbot commented 2 years ago

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

samhforbes commented 2 years ago

Hi @elimillera, @spitschan, @angerhang please see the instructions at the top of the review thread.

Once you've generated your checklist you can use this as the basis for your review. Generally speaking it's useful to post an overview here, but deal with individual points by opening issues on the target repository, and linking them here so we are all on the same page. Feel free to direct any queries to me, but otherwise we look forward to the benefit of your expertise.

angerhang commented 2 years ago

Review checklist for @angerhang

Conflict of interest

[x] 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

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

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

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] 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?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] 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?

angerhang commented 2 years ago

Overall, I found activAnalyzer easy to navigate and use. The documentation and paper are well-written and easy to follow. So happy to see another open source software trying to reduce the hurdle of the costly ActiLife software in the community. The added integration of COPD questionnaires is excellent. I am happy to tick most of the checkboxes above with a few minor comments:

Limited gender category in the survey UI
Good to clarify high-level dependencies for both debugging and others to contribute in the future
It would also be good to provide a sample .agd file for us so that can verify that the package works? It might also be a good idea to include this test file in your CI tests to ensure that your data gets processed properly.

@pydemull thanks a lot for making this valuable contribution to the community. I look forward to your revisions and seeing the generated report myself using some of the test files before signing off the rest of the checklist.

Great job, nontheless :D

elimillera commented 2 years ago

Review checklist for @elimillera

Conflict of interest

[x] 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

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

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

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] 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?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] 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?

pydemull commented 2 years ago

Thank you very much @angerhang for your comments. Before making the required changes to the package, I have some questions to be sure of what you want.

Regarding the issue related to the "gender" options: Actually, it was not "Gender" (that refers to a "constellation of sociocultural processes that interact with and have the potential to influence human biology" [Schiebinger, et al. Lancet. 2016]) but "Sex" (that refers to "a biological variable based upon chromosomal assignment, and generally male, female, or intersex" [Schiebinger, op. cit.]) that should be provided. While I totally respect and am happy with all the gender categories, there are two reasons for which I choose sex information only. The first one is that the equations used in the app (and that have been previously published) actually use a binary classification (male/female, men/women) and I suspect that the studies that used the "gender" term when they did not use the "sex" term, actually took (reported) sex information, because there are of course more than only two gender categories. The second reason is that gender may be more complex to assess, at least when reported (but I may be wrong when saying this), and because the app did not need this information, I prefered to avoid this difficulty. However, in the editorial by Schiebinger cited above, the term "intersex" is evocated, and this may be an information confirming that my options are indeed incomplete. Would it be ok for you if I keep the "sex" information with the following options: "male", "female", "intersex", "undefined"?
Regarding your remarks for the .agd file: Actually, once the package is installed locally, you can access the file by the following code (as shown in the examples of the functions): file <- system.file("extdata", "acc.agd", package = "activAnalyzer"). This is the data file that is loaded when the user clicks on the dedicated button to choose the "demo file" inside the app. This also is this file that is used for my CI tests that are performed thanks to the files that can be found in the "tests" folder of the package. So yes, there are CI tests (or maybe you though about other things?). Based on this comment, would you prefer I make available the agd file content when the user uses the data() function? Or my explanations are now sufficient for you?

Thanks a lot for your time and work.

angerhang commented 2 years ago

Thank you very much @angerhang for your comments. Before making the required changes to the package, I have some questions to be sure of what you want.

Regarding the issue related to the "gender" options: Actually, it was not "Gender" (that refers to a "constellation of sociocultural processes that interact with and have the potential to influence human biology" [Schiebinger, et al. Lancet. 2016]) but "Sex" (that refers to "a biological variable based upon chromosomal assignment, and generally male, female, or intersex" [Schiebinger, op. cit.]) that should be provided. While I totally respect and am happy with all the gender categories, there are two reasons for which I choose sex information only. The first one is that the equations used in the app (and that have been previously published) actually use a binary classification (male/female, men/women) and I suspect that the studies that used the "gender" term when they did not use the "sex" term, actually took (reported) sex information, because there are of course more than only two gender categories. The second reason is that gender may be more complex to assess, at least when reported (but I may be wrong when saying this), and because the app did not need this information, I prefered to avoid this difficulty. However, in the editorial by Schiebinger cited above, the term "intersex" is evocated, and this may be an information confirming that my options are indeed incomplete. Would it be ok for you if I keep the "sex" information with the following options: "male", "female", "intersex", "undefined"?

Regarding your remarks for the .agd file: Actually, once the package is installed locally, you can access the file by the following code (as shown in the examples of the functions): file <- system.file("extdata", "acc.agd", package = "activAnalyzer"). This is the data file that is loaded when the user clicks on the dedicated button to choose the "demo file" inside the app. This also is this file that is used for my CI tests that are performed thanks to the files that can be found in the "tests" folder of the package. So yes, there are CI tests (or maybe you though about other things?). Based on this comment, would you prefer I make available the agd file content when the user uses the data() function? Or my explanations are now sufficient for you?

Thanks a lot for your time and work.

Thanks a lot for your response @pydemull . Now I understand the rationale of your design choice. Thanks for being willing to build a more inclusive scientific software. Perhaps intersex and undefined will work? I guess with non-binary individuals, you will just take an average of both sex for your calculation?
I think your explanations are sufficient. I will try testing the software using the sample data this weekend. Thanks a lot!

pydemull commented 2 years ago

Dear @samhforbes @angerhang @elimillera, CRAN has requested I make some updates of the package so that it passes R-dev checking (this is in relation with a planned update of R regarding as.character.POSIXt()). I have made the update and the current CRAN version is now 1.0.5. (Thus, I will have to make a change in the JOSS paper where I gave a version number for the package; I will remove the version number for the package). Sincerely

pydemull commented 2 years ago

Thank you very much @angerhang for your comments. Before making the required changes to the package, I have some questions to be sure of what you want.

Regarding the issue related to the "gender" options: Actually, it was not "Gender" (that refers to a "constellation of sociocultural processes that interact with and have the potential to influence human biology" [Schiebinger, et al. Lancet. 2016]) but "Sex" (that refers to "a biological variable based upon chromosomal assignment, and generally male, female, or intersex" [Schiebinger, op. cit.]) that should be provided. While I totally respect and am happy with all the gender categories, there are two reasons for which I choose sex information only. The first one is that the equations used in the app (and that have been previously published) actually use a binary classification (male/female, men/women) and I suspect that the studies that used the "gender" term when they did not use the "sex" term, actually took (reported) sex information, because there are of course more than only two gender categories. The second reason is that gender may be more complex to assess, at least when reported (but I may be wrong when saying this), and because the app did not need this information, I prefered to avoid this difficulty. However, in the editorial by Schiebinger cited above, the term "intersex" is evocated, and this may be an information confirming that my options are indeed incomplete. Would it be ok for you if I keep the "sex" information with the following options: "male", "female", "intersex", "undefined"?

Regarding your remarks for the .agd file: Actually, once the package is installed locally, you can access the file by the following code (as shown in the examples of the functions): file <- system.file("extdata", "acc.agd", package = "activAnalyzer"). This is the data file that is loaded when the user clicks on the dedicated button to choose the "demo file" inside the app. This also is this file that is used for my CI tests that are performed thanks to the files that can be found in the "tests" folder of the package. So yes, there are CI tests (or maybe you though about other things?). Based on this comment, would you prefer I make available the agd file content when the user uses the data() function? Or my explanations are now sufficient for you?

Thanks a lot for your time and work.

Thanks a lot for your response @pydemull . Now I understand the rationale of your design choice. Thanks for being willing to build a more inclusive scientific software. Perhaps intersex and undefined will work? I guess with non-binary individuals, you will just take an average of both sex for your calculation?

I think your explanations are sufficient. I will try testing the software using the sample data this weekend. Thanks a lot!

Good question for the calculation with intersex... I have not yet found any reference regarding energy expenditure for this. I must think about it...

samhforbes commented 2 years ago

@pydemull not to worry, we can set the version number both here and in the paper at the end.

Thanks everyone for your discussions and input to date!

angerhang commented 2 years ago

Thank you very much @angerhang for your comments. Before making the required changes to the package, I have some questions to be sure of what you want.

Regarding the issue related to the "gender" options: Actually, it was not "Gender" (that refers to a "constellation of sociocultural processes that interact with and have the potential to influence human biology" [Schiebinger, et al. Lancet. 2016]) but "Sex" (that refers to "a biological variable based upon chromosomal assignment, and generally male, female, or intersex" [Schiebinger, op. cit.]) that should be provided. While I totally respect and am happy with all the gender categories, there are two reasons for which I choose sex information only. The first one is that the equations used in the app (and that have been previously published) actually use a binary classification (male/female, men/women) and I suspect that the studies that used the "gender" term when they did not use the "sex" term, actually took (reported) sex information, because there are of course more than only two gender categories. The second reason is that gender may be more complex to assess, at least when reported (but I may be wrong when saying this), and because the app did not need this information, I prefered to avoid this difficulty. However, in the editorial by Schiebinger cited above, the term "intersex" is evocated, and this may be an information confirming that my options are indeed incomplete. Would it be ok for you if I keep the "sex" information with the following options: "male", "female", "intersex", "undefined"?

Regarding your remarks for the .agd file: Actually, once the package is installed locally, you can access the file by the following code (as shown in the examples of the functions): file <- system.file("extdata", "acc.agd", package = "activAnalyzer"). This is the data file that is loaded when the user clicks on the dedicated button to choose the "demo file" inside the app. This also is this file that is used for my CI tests that are performed thanks to the files that can be found in the "tests" folder of the package. So yes, there are CI tests (or maybe you though about other things?). Based on this comment, would you prefer I make available the agd file content when the user uses the data() function? Or my explanations are now sufficient for you?

Thanks a lot for your time and work.

Thanks a lot for your response @pydemull . Now I understand the rationale of your design choice. Thanks for being willing to build a more inclusive scientific software. Perhaps intersex and undefined will work? I guess with non-binary individuals, you will just take an average of both sex for your calculation?

I think your explanations are sufficient. I will try testing the software using the sample data this weekend. Thanks a lot!

Tested with the demo data. The report generated is super sick! Am sure many will benefit from using this software. There are still outstanding tasks regarding the sex calculation and dependency instructions. But I am happy to tick all the boxes from my checklist :D

pydemull commented 2 years ago

Thanks again for your comments! I have provided answers for the two issues you have opened. Regarding the dependency instructions, I am not sure your comment above was done before or after my answer on the dedicated GitHub issue. Please tell me if it is not enough. Best

samhforbes commented 2 years ago

Thanks @angerhang for the productive comments so far.

Hi @elimillera, @spitschan - while the last couple of comments from @angerhang are resolved, do let us know if there's any issues with completing the review, or let @pydemull know if there's any barriers to running anything for the review.

elimillera commented 2 years ago

Hey @pydemull, I've reviewed the package, repository, and paper. I've ran all the tests I generally run to determine package quality and they are all coming back green, they repository is well formed and has good CI and contributor information, and the paper is well written.

Looks good to me!

pydemull commented 2 years ago

Thanks @elimillera for your time and work. Glad to know that all seem ok for you. @samhforbes, @angerhang, for information I have merged the dev branch with the master branch in relation to the modifications made following the @angerhang's comments. I have then taken the liberty to close the issues opened by @angerhang. Best