search

openjournals / joss-reviews

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

[REVIEW]: Systole: A python package for cardiac signal synchrony and analysis #3832

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @LegrandNico (Nicolas Legrand) Repository: https://github.com/embodied-computation-group/systole/ Version: v0.2.2 Editor: @osorensen Reviewers: @axel-loewe, @janfreyberg Archive: 10.5281/zenodo.5787520

: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/0db9e2b393ac69b67ecd1a6d6c12ae87"><img src="https://joss.theoj.org/papers/0db9e2b393ac69b67ecd1a6d6c12ae87/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/0db9e2b393ac69b67ecd1a6d6c12ae87/status.svg)](https://joss.theoj.org/papers/0db9e2b393ac69b67ecd1a6d6c12ae87)

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

@axel-loewe and @janfreyberg, 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 @osorensen know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @axel-loewe

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @janfreyberg

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @axel-loewe 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
osorensen commented 3 years ago

@whedon generate pdf from branch dev

whedon commented 3 years ago 
Attempting PDF compilation from custom branch dev. Reticulating splines etc...
whedon commented 3 years ago

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

osorensen commented 3 years ago

@whedon add @janfreyberg as reviewer

whedon commented 3 years ago

OK, @janfreyberg is now a reviewer

osorensen commented 3 years ago

Sorry @janfreyberg, I had forgot to add you in the pre-review issue. Created your checklist manually now. Please let me know if it doesn't work.

axel-loewe commented 3 years ago

Overall, the manuscript is nicely written and already fulfills most quality criteria.

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation?

  • I had trouble to install systole because of the numba/numpy dependencies. It did not install via pip on different machines and operating systems (macOS and CentOS) with the following error: numba 0.54.1 requires numpy<1.21,>=1.17, but you'll have numpy 1.21.2 which is incompatible. Reported as https://github.com/embodied-computation-group/systole/issues/19

  • [x] Functionality: Have the functional claims of the software been confirmed? Cannot be evaluated if not installing.

  • [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.) Cannot be evaluated if not installing.

Documentation

  • [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

  • Dependencies are managed in a requirements.txt file and mentioned in the docs. 2 are listed in the requirements file but not on the web page: pyserial, setuptools

  • The dependencies also lead to a minimum required Python version of 3.7, which should be mentioned.

  • [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

  • The authors might want to check the images on website. Some on the front end did not load (lab logos).

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

  • The repository could benefit from a CONTRIBUTING file clearly outlining potential ways to contribute to systole. Issues seem to be collected in the GitHub issue tracker. This could be mentioned explicitly. I would also encourage the authors to clearly communicate via which channels users can ask for support. Ideally a platform that makes the answers available to other users as well.

  • [ ] State of the field: Do the authors describe how this software compares to other commonly-used packages?

  • As mentioned by the authors, systole is inspired by existing toolboxes. It would be helpful to spell out clearly what the unique features of systole are.

  • Along these lines, it would also be helpful to compare systole against other software solutions.

  • [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?

  • L.43: Use \cite instead of \citep

osorensen commented 3 years ago

@LegrandNico, can you please address the points raised by @axel-loewe in the post above, and report on the progress here?

LegrandNico commented 3 years ago

Thank you for reviewing this package, and sorry for the delay in providing fixes for the issues I have been on holiday for the last two weeks.

I have moved all the modifications and the paper to the master branch to simplify the review process. The package can be installed for the master branch, but it is not updated yet on pip as it might be better to wait for the final version (?).

I have committed modifications for some of the comments regarding the package dependencies and the documentation, I still need to work on @axel-loewe 's Community guidelines and State of the field, I will do this as soon as possible

osorensen commented 3 years ago

Thanks @LegrandNico. Just wanted to comment that I agree that it makes sense to wait with updating pip until the final version.

whedon commented 3 years ago

:wave: @axel-loewe, please update us on how your review is going (this is an automated reminder).

janfreyberg commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

janfreyberg commented 3 years ago

All software-related review items look great to me now - the software installs and runs as documented. Pending the state of the field addition to the paper, all looks good to me :)

One point: I was unable to run the code related to online signal processing, as it requires hardware. This code is also not tested automatically. However, given the nature of the code, I don't think it's possible to change that without extensive mocking, which I think would be pointless, so happy to leave this untested!

axel-loewe commented 3 years ago

I had to install requests as an additional dependency that was not listed.

Also, I had to add a couple of imports in order to run the Getting Started examples (e.g., from systole.plots plot_raw)

osorensen commented 3 years ago

Thanks to both @axel-loewe and @janfreyberg. @LegrandNico, please ping us in this thread when you have fixed the remaining issues raised by the reviewers in their last two comments.

LegrandNico commented 3 years ago
axel-loewe commented 3 years ago
  • Regarding the inspiration from other toolboxes, Systole is primarily made from code that was developed in the lab from scratch to implement new or preexisting algorithms, in which case the reference to the original article is mentioned in the docstring. Most of the time, no Python code was copied/duplicated. One main exception is the re-implementation of the ECG peaks detection algorithms from the py-ecg-detectors package. This package was previously part of the dependencies but we finally re-implemented the code directly in Systole to allow acceleration using Numba. We try to mention and credit this as much as possible in the docstrings, tutorial and example. It could maybe be more emphasised in the paper? Also, while we want to make this import explicit, it is also worth noting that it is not an identical copy, but that the functions have been significantly refactored and performance improved.
  • We can run some comparisons with other software, the most straightforward one would be performance comparison on feature detection and artefact correction with other software (e.g. mne, neurokit). Is there a specific analysis/dataset we should be used in particular?

Both these comments were more aiming at a clear description what the unique features of systole are, i.e. why a user might want to chose systole rather than one of the alternative solutions.

osorensen commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

osorensen commented 3 years ago
  • Regarding the inspiration from other toolboxes, Systole is primarily made from code that was developed in the lab from scratch to implement new or preexisting algorithms, in which case the reference to the original article is mentioned in the docstring. Most of the time, no Python code was copied/duplicated. One main exception is the re-implementation of the ECG peaks detection algorithms from the py-ecg-detectors package. This package was previously part of the dependencies but we finally re-implemented the code directly in Systole to allow acceleration using Numba. We try to mention and credit this as much as possible in the docstrings, tutorial and example. It could maybe be more emphasised in the paper? Also, while we want to make this import explicit, it is also worth noting that it is not an identical copy, but that the functions have been significantly refactored and performance improved.
  • We can run some comparisons with other software, the most straightforward one would be performance comparison on feature detection and artefact correction with other software (e.g. mne, neurokit). Is there a specific analysis/dataset we should be used in particular?

Both these comments were more aiming at a clear description what the unique features of systole are, i.e. why a user might want to chose systole rather than one of the alternative solutions.

@LegrandNico, could you please address this point raised by @axel-loewe and notify us here once the manuscript has been updated?

osorensen commented 3 years ago

@axel-loewe, there are a couple of unchecked boxed in your review checklist at the top of this thread, in regarding Functionality and Documentation. Could you please check if these have been addressed the authors, and if so, update the checklist? If your these issues have not been sufficiently addressed, please suggest what remains to be done.

LegrandNico commented 2 years ago

I have added a sentence in the manuscript to clarify the unique features of the package.

If this one is accepted, I think I have fixed all the issues that were raised by the reviewers.

Just let me know if I can do anything to improve the package/manuscript.

osorensen commented 2 years ago

Thanks @LegrandNico.

@axel-loewe, can you please see if your points have been addressed, and if so, update the checklist at the top of this thread?

osorensen commented 2 years ago

@whedon generate pdf

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

axel-loewe commented 2 years ago

All good from my end!

osorensen commented 2 years ago

@whedono check references

whedon commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
osorensen commented 2 years ago

@whedon check references

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/MCSE.2007.55 is OK
- 10.1101/2020.06.10.144428 is OK
- 10.1101/2021.02.18.431871 is OK
- 10.5281/ZENODO.3353396 is OK
- 10.1016/j.trf.2019.09.015 is OK
- 10.1080/03091902.2019.1640306 is OK
- 10.3758/s13428-020-01516-y is OK
- 10.3758/s13428-018-01193-y is OK

MISSING DOIs

- None

INVALID DOIs

- None
osorensen commented 2 years ago

@LegrandNico, I have created some issues in the source repository pointing out a few things that need to be corrected before I can recommend accept. Also, can you please

osorensen commented 2 years ago

@LegrandNico, any news on this? I'm ready to suggest acceptance once you've fixed the issues I pointed out in the source repository and completed the points in my previous post.

LegrandNico commented 2 years ago

I have just release v0.2.2. The list of version tags is the following:

The DOI for the last (0.2.2) Zenodo version is 10.5281/zenodo.5787520

@micahgallen can you check the Zenodo metadata and let us know when this is ready?

osorensen commented 2 years ago

@whedon set doi as 10.5281/zenodo.5787520

whedon commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
osorensen commented 2 years ago

@whedon set 10.5281/zenodo.5787520 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5787520 is the archive.

osorensen commented 2 years ago

@whedon set v0.2.2 as version

whedon commented 2 years ago

OK. v0.2.2 is the version.

osorensen commented 2 years ago

@LegrandNico, the title, author list, and version of the Zenodo archive match those of the submission, so I'm ready to recommend accept here. Is this ok with you?

LegrandNico commented 2 years ago

Yes we made all the changes I think we are ready

osorensen commented 2 years ago

@whedon recommend-accept

whedon commented 2 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/MCSE.2007.55 is OK
- 10.1101/2020.06.10.144428 is OK
- 10.1101/2021.02.18.431871 is OK
- 10.5281/ZENODO.3353396 is OK
- 10.1016/j.trf.2019.09.015 is OK
- 10.1080/03091902.2019.1640306 is OK
- 10.3758/s13428-020-01516-y is OK
- 10.3758/s13428-018-01193-y is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 2 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/2847

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

@whedon accept deposit=true
arfon commented 2 years ago

@LegrandNico – could you please make sure to cite Scipy, Numpy, Pandas and any other package you mention in your paper please?

LegrandNico commented 2 years ago

I have added the missing citations for Numpy, SciPy and Pandas where they are mentioned in the summary. For Pandas I used the template provided on the website but I think the author list (The pandas development team) is not rendered correctly in the compiled paper.

arfon commented 2 years ago

@whedon recommend-accept