openjournals / joss-reviews

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

[REVIEW]: Sit2StandPy: An Open-Source Python Package for Detecting and Quantifying Sit-to-Stand Transitions Using an Accelerometer on the Lower Back' #2449

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @LukasAdamowicz (Lukas Adamowicz) Repository: https://github.com/PfizerRD/sit2standpy Version: v1.1.2 Editor: @galessiorob Reviewer: @jsgalan, @gweissman Archive: 10.5281/zenodo.3988351

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@jsgalan & @gweissman, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @galessiorob know.

Please try and complete your review in the next six weeks

Review checklist for @jsgalan

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @gweissman

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jsgalan, @gweissman it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago
Reference check summary:

OK DOIs

- 10.1088/0967-3334/33/11/1947 is OK
- 10.1016/j.jbiomech.2009.06.023 is OK
- 10.1186/s12984-015-0090-9 is OK
- 10.1016/j.gaitpost.2012.10.008 is OK
- 10.1186/s12984-017-0241-2 is OK
- 10.1186/s12984-015-0026-4 is OK
- 10.3389/fneur.2018.00652 is OK
- 10.1016/j.patrec.2018.03.020 is OK
- 10.1093/geronj/49.2.m85 is OK
- 10.1109/MCSE.2011.37 is OK
- 10.21105/joss.01237 is OK
- 10.5281/zenodo.3898987 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

LukasAdamowicz commented 4 years ago

Hi all, Thank you for the comments/work so far - DOIs were added and look like they are showing up, I'll look into making sure that the file is packed with the installation @jsgalan

LukasAdamowicz commented 4 years ago

@jsgalan you should be able to now re-install from pip (make sure you get version 1.1.1.post4), which should contain the files needed for the tests (also made changes so that all the tests that were supposed to run are found correctly)

gweissman commented 4 years ago

I was able to install the package easily without apparent errors. To assess functionality, I ran the accompanying tests using pytest and all passed. I did not import the library directly or try running any of my own code with it.

One scientific concern is that the validation of the algorithms implemented in this software package appear to have been reported in a paper that is currently under review. Therefore, the scientific validity of the algorithms can't be assessed here. If that is important for JOSS, then I would suggest pending this review until those results have passed through some peer review process.

One ethical concern is that the repo appears at the Pfizer site, and I did not see any statement of funding regarding the research or disclosure about the involvement of the funding source in the development, testing, or reporting of the software or associated manuscript.

LukasAdamowicz commented 4 years ago

Thank you for your comments, @gweissman.

Regarding your last point, I added the following statement to the paper, which has been on the other papers our group has published (GaitPy, SleepPy)

Acknowledgements

The Digital Medicine & Translational Imaging group at Pfizer, Inc supported the development of this package.

galessiorob commented 4 years ago

Thank you so much for your review @gweissman! ⚡ I am looking into policies regarding this comment, I'll update the issue with my findings.

@jsgalan checking in, I know you started the review back in the pre-review issue, feel free to copy over your comments and start checking off the list at the top assigned to you. I tried installing today and didn't run into any issues.

Let me know if I can help with anything and thanks again for your work.

jsgalan commented 4 years ago

Hi all, I proceed to clone the git

Screen Shot 2020-07-14 at 10 37 33 AM

Then created a Venv

Screen Shot 2020-07-14 at 11 03 38 AM

Then did an initial install try (Python 2.7)

Screen Shot 2020-07-14 at 11 04 09 AM

Then installed pip

Screen Shot 2020-07-14 at 11 04 26 AM

Then a second install with python 3.8

Screen Shot 2020-07-14 at 11 04 47 AM

Then did a manual installation of the requierements

Screen Shot 2020-07-14 at 11 06 26 AM

Then installed Pytest

Screen Shot 2020-07-14 at 11 05 59 AM

then ran the tests

V1

Screen Shot 2020-07-14 at 10 47 11 AM

V2

Screen Shot 2020-07-14 at 10 47 33 AM

All passed without problems!

Then went and did ran the code in the repository for V2 usage and old usage (here called test2 and test3, respectively)

87450282-ed5fe200-c5c3-11ea-8e53-97132204df06 Screen Shot 2020-07-14 at 11 18 44 AM0-2

I try in my editor, just after a fresh pip install

Screen Shot 2020-07-14 at 11 24 18 AM

and got the same errors:

Screen Shot 2020-07-14 at 11 24 26 AM Screen Shot 2020-07-14 at 11 24 39 AM

Not sure what is going wrong of if I am missing a step somewhere.

Best

LukasAdamowicz commented 4 years ago

@jsgalan thank you for finding those issues. I've addressed both of them with the usage examples, which have been updated in the repository README. In summary, there was a typo in the V1 (old usage) example, which should be fixed now:

... with resources.path('sit2standpy.data', 'sample.csv') as file_path: ...

Regarding V2 usage, data_transform_function was intended to be the users own function for achieving the required format. The comments around this have been updated, and I added a link to a gist as well, that provides a simple data loader function

data loader GIST

jsgalan commented 4 years ago

Hi @LukasAdamowicz,

V1 works fine

Screen Shot 2020-07-14 at 12 12 20 PM

V2 using the loader as suggested give me an error

Screen Shot 2020-07-14 at 12 27 35 PM

After an small inspection I realized there was an error is 'Unix Time' instead of 'Unix Timestamps' (I know it is kind off-topic since is just an example)

Screen Shot 2020-07-14 at 12 56 46 PM

After that correction I get to this error, not sure why?

Screen Shot 2020-07-14 at 12 49 57 PM

Best

LukasAdamowicz commented 4 years ago

@jsgalan thank you, apologies for these difficulties. I've gone an updated the README with the v2 usage (tested on my end) - basically what was missing was the library prefix for the WindowDays (and following calls) that got added to the Sequence, so it becomes

...
sequence.add(s2s.v2.WindowDays(hours=[8, 20]))
sequence.add(s2s.v2.AccelerationFilter())
sequence.add(s2s.v2.Detector(stillness_constraint=True)) 
...

Also please note that path_to_csv_output is undefined, and would need to be defined as well to get the code working fully

I also fixed the gist as well to correctly use the correct key names

jsgalan commented 4 years ago

Hi @LukasAdamowicz

It worked!

Screen Shot 2020-07-14 at 3 11 29 PM

jsgalan commented 4 years ago

HI about the written articles, I am not sure I am seeing the last version. Since I am not seeing the acknowledgment you mentioned earlier.

Thank you for your comments, @gweissman.

Regarding your last point, I added the following statement to the paper, which has been on the other papers our group has published (GaitPy, SleepPy)

Acknowledgements

The Digital Medicine & Translational Imaging group at Pfizer, Inc supported the development of this package.

Also I think this can be easily solved, just adding a line to the code.

Screen Shot 2020-07-14 at 3 13 53 PM

As @gweissman has made me notice in his review, I think the authors should give a few more details on other (open souse) software implementations, or state that is a novel implementation, to be able to answer this State of the field: Do the authors describe how this software compares to other commonly-used packages?

LukasAdamowicz commented 4 years ago

The paper is updated with the acknowledgements, just no new pdf preview yet.

LukasAdamowicz commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

jsgalan commented 4 years ago

Hi Lukas, Please check the Availability Section seems to have the same error I noted earlier.

87472476-99fe8b80-c5e5-11ea-9add-2fde22e3cf4e

LukasAdamowicz commented 4 years ago

Fixed. I also added a statement regarding existing implementations/software of sit-to-stand algorithms

LukasAdamowicz commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

jsgalan commented 4 years ago

Hi all, I just checked the last version of the article and am satisfied with the results.

I am happy to accept the submission.

Best,

LukasAdamowicz commented 4 years ago

Thank you so much for your review @gweissman! ⚡ I am looking into policies regarding this comment, I'll update the issue with my findings.

@jsgalan checking in, I know you started the review back in the pre-review issue, feel free to copy over your comments and start checking off the list at the top assigned to you. I tried installing today and didn't run into any issues.

Let me know if I can help with anything and thanks again for your work.

Hi, is there any update on this @galessiorob @gweissman?

Thanks!

galessiorob commented 4 years ago

Hey @LukasAdamowicz

I talked to other editors and the consensus was that it's okay to publish the paper despite it citing an algorithm that is still not published as long as:

  1. You can confirm that the other publication for which you are also an author (Assessment of sit to stand during daily life using an accelerometer on the lower back. IEEE Journal of Biomedical and Health Informatics) is (at least partly) focused on determining the validity/applicability of the algorithm for this area of research.
  2. You can confirm this other paper is not simply about the software as well, as this might constitute self-plagiarism.
  3. Finally, adding some complementary documentation around the algorithm itself:
    • State what the algorithm is in the documentation.
    • Demonstrate that their implementation is verifiable through tests.
    • Discuss the role the other manuscript plays in vetting the application of the algorithm.

Let me know if you have any questions!

galessiorob commented 4 years ago

Hey @LukasAdamowicz wanted to check in, do you have any questions, is there anything I can do to help get your paper across the finish line?

LukasAdamowicz commented 4 years ago

Only some clarification for now - for the third point - is that documentation in the paper, or as part of the package documentation?

galessiorob commented 4 years ago

@LukasAdamowicz on the paper, thanks for asking!

galessiorob commented 4 years ago

@gweissman I noticed that you haven't checked off the State of the field item from your list, the paper states that "to the author's knowledge there are no commonly used open-source, and easy to use and configure, implementations of sit-to-stand algorithms available", do you think this needs to be expanded?

gweissman commented 4 years ago

@gweissman I noticed that you haven't checked off the State of the field item from your list, the paper states that "to the author's knowledge there are no commonly used open-source, and easy to use and configure, implementations of sit-to-stand algorithms available", do you think this needs to be expanded?

I will check it off. I think that is sufficient although I am not knowledgeable in this field to know how true that statement is. A quick web search seems to confirm that while numerous algorithms have been published, the software does not seem to be readily available.

LukasAdamowicz commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

LukasAdamowicz commented 4 years ago

Hi @galessiorob, After a discussion with my co-author, we decided instead to remove the citation to the pending paper.

Let me know if there is anything else to do now, at this point.

LukasAdamowicz commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

galessiorob commented 4 years ago

@whedon check references

whedon commented 4 years ago
Reference check summary:

OK DOIs

- 10.1088/0967-3334/33/11/1947 is OK
- 10.1016/j.jbiomech.2009.06.023 is OK
- 10.1186/s12984-015-0090-9 is OK
- 10.1016/j.gaitpost.2012.10.008 is OK
- 10.1186/s12984-017-0241-2 is OK
- 10.1186/s12984-015-0026-4 is OK
- 10.3389/fneur.2018.00652 is OK
- 10.1016/j.patrec.2018.03.020 is OK
- 10.1093/geronj/49.2.m85 is OK
- 10.1109/MCSE.2011.37 is OK
- 10.21105/joss.01237 is OK
- 10.5281/zenodo.3898987 is OK

MISSING DOIs

- None

INVALID DOIs

- None
galessiorob commented 4 years ago

@LukasAdamowicz I think leaving the pending paper citation out is fine, at this point the next thing we need from you is to deposit your software in Zenodo and post the DOI here. After that, we can set the final version, generate a final proof and have an Editor in Chief do the publishing.

LukasAdamowicz commented 4 years ago

https://zenodo.org/record/3988351

DOI: 10.5281/zenodo.3988351

galessiorob commented 4 years ago

@whedon set v1.1.2 as version

whedon commented 4 years ago

OK. v1.1.2 is the version.

galessiorob commented 4 years ago

@whedon set 10.5281/zenodo.3988351 as archive

whedon commented 4 years ago

OK. 10.5281/zenodo.3988351 is the archive.

galessiorob commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

galessiorob commented 4 years ago

@whedon accept

whedon commented 4 years ago
Attempting dry run of processing paper acceptance...
whedon commented 4 years ago
Reference check summary:

OK DOIs

- 10.1088/0967-3334/33/11/1947 is OK
- 10.1016/j.jbiomech.2009.06.023 is OK
- 10.1186/s12984-015-0090-9 is OK
- 10.1016/j.gaitpost.2012.10.008 is OK
- 10.1186/s12984-017-0241-2 is OK
- 10.1186/s12984-015-0026-4 is OK
- 10.3389/fneur.2018.00652 is OK
- 10.1016/j.patrec.2018.03.020 is OK
- 10.1093/geronj/49.2.m85 is OK
- 10.1109/MCSE.2011.37 is OK
- 10.21105/joss.01237 is OK
- 10.5281/zenodo.3898987 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/1648

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1648, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
galessiorob commented 4 years ago

@LukasAdamowicz could you please give this last version a final proofread? An editor in chief will take it from here and get it published now ✨

LukasAdamowicz commented 4 years ago

@galessiorob Paper looks good in proofread. Thanks for your time!