openjournals / joss-reviews

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

[REVIEW]: SleepECG: a Python package for sleep staging based on heart rate #5411

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@cbrnr<!--end-author-handle-- (Clemens Brunner) Repository: https://github.com/cbrnr/sleepecg Branch with paper.md (empty if default branch): joss Version: v0.5.5 Editor: !--editor-->@emdupre<!--end-editor-- Reviewers: @tompollard, @richrobe, @peterakirk Archive: 10.5281/zenodo.7993717

Status

status

Status badge code:

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

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

@tompollard & @richrobe & @peterakirk, 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 @emdupre 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 @richrobe

📝 Checklist for @peterakirk

📝 Checklist for @tompollard

editorialbot commented 1 year 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 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1111/j.1469-8986.1966.tb02650.x is OK
- 10.1109/TBME.1985.325532 is OK
- 10.1161/01.CIR.93.5.1043 is OK
- 10.5664/jcsm.26814 is OK
- 10.3389/fpubh.2017.00258 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.13 s (568.0 files/s, 191457.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
SVG                              8              0            109          17772
Python                          32            901           1611           3377
Markdown                        19            189              0            601
C                                1             67            134            360
YAML                             8             40              4            280
HTML                             3              0              0             55
TeX                              1              5              0             48
TOML                             1             12              0             24
JavaScript                       1              2              0             15
CSS                              2              1              0             10
-------------------------------------------------------------------------------
SUM:                            76           1217           1858          22542
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 720

editorialbot commented 1 year ago

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

emdupre commented 1 year ago

👋 Hi @tompollard, @richrobe, and @peterakirk, and thank you again for agreeing to review this submission !

The review will take place in this issue, and you can generate your individual reviewer checklists by asking editorialbot directly with @editorialbot generate my checklist.

In working through the checklist, you're likely to have specific feedback on SleepECG. Whenever possible, please open relevant issues on the linked software repository (and cross-link them with this issue) rather than discussing them here. This helps to make sure that feedback is translated into actionable items to improve the software !

If you aren't sure how to get started, please see the Reviewing for JOSS guide -- and, of course, feel free to ping me with any questions !

cbrnr commented 1 year ago

Looking forward to your comments and reviews! Pinging @hofaflo, who implemented the bulk of SleepECG (and who is therefore of course also an author of the submitted paper)!

richrobe commented 1 year ago

Review checklist for @richrobe

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

emdupre commented 1 year ago

@tompollard and @peterakirk, I just wanted to check-in as I noticed that you haven't yet generated your reviewer checklists. Please let me know if you're having any trouble accessing these !

Happy Thursday everyone, and May the Fourth be with you 😄

peterakirk commented 1 year ago

Review checklist for @peterakirk

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

emdupre commented 1 year ago

:wave: Hi @peterakirk ! I notice that you have completed your checklist but have not opened any issues on the software repository. Is there another place that you're providing feedback to the authors ? Please let me know !

peterakirk commented 1 year ago

Hi @emdupre, apologies, I've gone through the checklist but had other stuff come up, so had to put it on pause for the moment. As of yet, I have had no issues: everything has installed fine, runs smoothly, and documentation looks good. Just remaining is I'll make sure all the basic metrics look appropriate, which I'll try and get to next week.

richrobe commented 1 year ago

Hi @cbrnr,

thank you for putting together such a lovely package! In general, I like it very much and see it as a valuable contribution to sleep staging estimation from ECG data. After having gone through code, documentation and paper I'll summarize my thoughts below and have created extra issues in the package repository.

General Remarks

See also

Paper

The paper is easy written, easy to follow, and nicely outlines the need of the SleepECG package. It provides a good motivation, especially regarding the reproducibility of the existing approaches. Just some minor comments:

See also

Installation

See also

Examples

See also

Other than that, I have nothing to add. Let me know what you think of my suggestions! 🙂

tompollard commented 1 year ago

emdupre I am looking at this today! Sorry it's taken me a while to get to it.

tompollard commented 1 year ago

Review checklist for @tompollard

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

cbrnr commented 1 year ago

@richrobe thank you so much for your detailed comments! I've already started addressing them in the issues you created!

tompollard commented 1 year ago

Hi @cbrnr -

I agree with @richrobe. sleepecg is a helpful, well-described, easy to use software package. I added some thoughts below, but I also think that the software is in great shape as it stands. My comments mostly relate to ongoing maintenance tasks rather than things that need to be dealt with right now.

While I looked over the code (which is cleaner than 99% of code I come across, and certainly all of mine!) I didn't do a deep dive into any of the algorithms. As the package provides a framework for supporting a breadth of algorithms and does not claim to implement successful, validated sleep staging (at least right now), this didn't seem especially necessary.

Installation

Dependencies

For development I'd usually expect to see a file containing the dependencies somewhere (e.g. requirements.txt or environment.yml). Does this exist? I see that the dependencies are listed in README.md, but it would be nice to have a file that can be used by package managers (e.g. pip, conda).

pip

pip install sleepecg correctly installs sleepecg-0.5.4 on my machine (Apple M1 Max, macOS Ventura 13.3, Python 3.10, pip 23.0).

As mentioned by @richrobe in https://github.com/cbrnr/sleepecg/issues/158, pip install sleepecg[full] displays a no matches found: sleepecg[full] error in zshell.

pip install 'sleepecg[full]' correctly installs sleepecg-0.5.4 (full) on my machine.

import

When importing sleepecg (v0.5.4), I see the following deprecation notices:

>>> import sleepecg
/Users/tompollard/sand/sleepecg/env/lib/python3.10/site-packages/sleepecg/heartbeats.py:638: NumbaDeprecationWarning: The 'nopython' keyword argument was not supplied to the 'numba.jit' decorator. The implicit default value for this argument is currently False, but it will be changed to True in Numba 0.59.0. See https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-object-mode-fall-back-behaviour-when-using-jit for details.
  _squared_moving_integration_numba = jit(_squared_moving_integration_py)
/Users/tompollard/sand/sleepecg/env/lib/python3.10/site-packages/sleepecg/heartbeats.py:639: NumbaDeprecationWarning: The 'nopython' keyword argument was not supplied to the 'numba.jit' decorator. The implicit default value for this argument is currently False, but it will be changed to True in Numba 0.59.0. See https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-object-mode-fall-back-behaviour-when-using-jit for details.
  _thresholding_numba = jit(_thresholding_py)

It looks like this is already addressed in the repo, so I haven't raised an issue: https://github.com/cbrnr/sleepecg/blob/d3d15d5bfa6386bdc68fb3c9bfc58d8da5f50d01/sleepecg/heartbeats.py#L638-L640

Code

Relative vs absolute imports

Absolute imports seem to be generally preferred by Python developers (https://peps.python.org/pep-0008/#imports). sleepecg uses relative imports in places (e.g. from .physionet import _list_physionet). Is this intentional? If not, perhaps switch to absolute imports at some point?

Scripts

The scripts (e.g. examples/benchmark/plot_benchmark_results.py) are missing structure that I think can be helpful (e.g. if __name__ == "__main__":). Minor thing and not necessary, but might be worth adding some of this structure (e.g. to prevent them running when imported)? This is the outline I use: https://www.annasyme.com/docs/python_structure.html.

Command line interface

Adding a command line interface for the package via pyproject.toml might be a nice addition, though not necessary.

scipy.misc.electrocardiogram has been deprecated

The example code at https://sleepecg.readthedocs.io/en/stable/heartbeat_detection/#usage uses scipy.misc.electrocardiogram which has been deprecated. Needs updating to scipy.datasets.electrocardiogram at some point.

ecg = electrocardiogram()

<stdin>:1: DeprecationWarning: scipy.misc.electrocardiogram has been deprecated in SciPy v1.10.0; and will be completely removed in SciPy v1.12.0. Dataset methods have moved into the scipy.datasets module. Use scipy.datasets.electrocardiogram instead.

Paper

Comments on the paper at: https://github.com/openjournals/joss-papers/blob/joss.05411/joss.05411/10.21105.joss.05411.pdf

Figure 1

I'm not sure I understand the Y-axis labels on Figure 1. Could you add details to either the axis label or the caption? The caption of axis label should highlight the log scale. LTDB should be defined somewhere in the text or caption somewhere. The algorithms should also be defined (e.g. it is unclear to me what "mne" means).

Add heartbeat detection metrics figure?

There is a metrics figure at https://sleepecg.readthedocs.io/en/stable/heartbeat_detection/#usage that I think would be a nice addition to the paper.

Add sleep staging figure?

Given the focus of the package is sleep staging, I think it would be nice to add a figure demonstrating the sleep staging functionality. A (ideally non-cheat!) version of the hypnogram at https://sleepecg.readthedocs.io/en/stable/plot/ might be a good one to include?

State of field

I'm not familiar with the literature on sleep-staging with ECG, but I do see there are one or two open source codebases that claim to enable ecg-based sleep staging. I wonder whether these should be briefly mentioned?

e.g. should comparisons be made to https://github.com/bdsp-core/ecg_respiration_sleep_staging? The repo doesn't look completely baked (e.g. "training_code_still_messy)", so perhaps it reinforces your point that "either the processing pipeline or the dataset used to generate the published results are not publicly available."

Reiterate point that sleep staging is not a solved problem?

I think it would be good to reiterate the point that sleep staging is not a solved problem. This is mentioned in the paper, but I think it is helpful for the user to be reminded that the package is not claiming (currently) to be able to successfully stage sleep with ECG.

References

Not completely necessary, but ideally I think the algorithms and datasets should be formally cited in the paper if they are being discussed. This helps to credit the creators and also supports the idea of reproducibility.

Archival version of the code

It wasn't clear to me whether there is an archival version of the software anywhere. If not, it would be great if could archive a copy (with DOI) somewhere like Zenodo or Figshare. I have a conflict of interest, but we would also be happy to host the software on PhysioNet.

Documentation

import numpy as np should be moved to the code block above at https://sleepecg.readthedocs.io/en/stable/heartbeat_detection/#usage (to avoid rri = 1000 * np.diff(beats) / fs failing with an import error).

cbrnr commented 1 year ago

Thank you @tompollard, these are great and thoughtful comments! It will take me some time to process and address each of them, and I'll be using this thread here and/or the issues you opened!

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

cbrnr commented 1 year ago

@tompollard I've addressed most of your comments in the corresponding issues, so here I'm only going to reply to points where this is not the case (but I'll indicate the issues where relevant).

tompollard commented 1 year ago

@cbrnr thanks for your quick response. @emdupre as far as I'm concerned, the code and paper look great and they are fine to publish in their current state!

cbrnr commented 1 year ago

Thanks @tompollard, I do want to include pointers to related repos (I need to check them out first) and a hypnogram. I'll report back once this is done.

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

cbrnr commented 1 year ago

I think I've addressed all comments. The latest paper version is linked above. Let me know if there is anything else!

peterakirk commented 1 year ago

Hi all,

Firstly, well done to @cbrnr and @hofaflo for curating what appears to be a great package. I’m not as familiar with the sleep literature, but classifying sleep stages from ECG is not an approach I had heard of before. This package seems like a great way to make this accessible to the field, in a reproducible and open manner.

I’ve not too much to add. I agree with the other reviewer comments, the most of which seem to have already been addressed by the authors. Below are just a few quick comments (the latter of which I've opened as issues):

RMSSD_benchmark

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

peterakirk commented 1 year ago

@emdupre, the authors have addressed my comments and everything looks good to me.

emdupre commented 1 year ago

🙏 Thank you @peterakirk, @tompollard, and @richrobe for your thoughtful reviews -- and @cbrnr for your engagement throughout this process !

I'll perform a few editorial checks now and update on those shortly.

emdupre commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1111/j.1469-8986.1966.tb02650.x is OK
- 10.1109/TBME.1985.325532 is OK
- 10.1161/01.CIR.93.5.1043 is OK
- 10.5664/jcsm.26814 is OK
- 10.3389/fpubh.2017.00258 is OK

MISSING DOIs

- None

INVALID DOIs

- None
emdupre commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

cbrnr commented 1 year ago

The version number to be used in the JOSS paper is v0.5.5. The Zenodo DOI is 10.5281/zenodo.7993717.

emdupre commented 1 year ago

Thank you, @cbrnr !

I've finished reviewing the code and software and am also very happy with the state of the submission. I did have a few editorial requests :

emdupre commented 1 year ago

@editorialbot set v0.5.5 as version

editorialbot commented 1 year ago

Done! version is now v0.5.5

emdupre commented 1 year ago

@editorialbot set 10.5281/zenodo.7993717 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.7993717

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

cbrnr commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

cbrnr commented 1 year ago

@emdupre done. I've used {{ }} for titles to ensure that the original casing is used, but I've found and corrected one error. Now all titles should be consistent and correct.

cbrnr commented 1 year ago

@editorialbot generate pdf