NeuroKit icon indicating copy to clipboard operation
NeuroKit copied to clipboard

[Feature] Report describing methods in PPG process

Open danibene opened this issue 1 year ago • 2 comments

See https://github.com/neuropsychology/NeuroKit/issues/665

Description

This PR aims to add the option to output an HTML report describing the methods used to process a PPG signal.

Proposed Changes

I changed the ppg_process() function so that the user can optionally output a report by specifying the name of the report file with the argument report.

(I'm not properly trained in software development best practices so if you think the code could be written in a clearer way, feel free to make suggestions!)

Checklist

  • [x] I have read the CONTRIBUTING file.
  • [x] My PR is targetted at the dev branch (and not towards the master branch).
  • [x] I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • [ ] I have added the newly added features to News.rst (if applicable)

danibene avatar Sep 13 '22 23:09 danibene

Codecov Report

Base: 52.74% // Head: 53.16% // Increases project coverage by +0.41% :tada:

Coverage data is based on head (9e01ff6) compared to base (b72c7e1). Patch coverage: 77.30% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #707      +/-   ##
==========================================
+ Coverage   52.74%   53.16%   +0.41%     
==========================================
  Files         279      282       +3     
  Lines       12624    12772     +148     
==========================================
+ Hits         6659     6790     +131     
- Misses       5965     5982      +17     
Impacted Files Coverage Δ
neurokit2/ppg/ppg_clean.py 75.00% <ø> (+18.75%) :arrow_up:
neurokit2/ppg/ppg_intervalrelated.py 16.21% <0.00%> (ø)
neurokit2/ppg/ppg_simulate.py 95.00% <ø> (ø)
neurokit2/ppg/ppg_plot.py 48.00% <42.85%> (+34.20%) :arrow_up:
neurokit2/ppg/ppg_methods.py 84.90% <84.90%> (ø)
neurokit2/ppg/ppg_process.py 95.45% <90.00%> (-4.55%) :arrow_down:
neurokit2/misc/report.py 96.15% <96.15%> (ø)
neurokit2/ppg/__init__.py 100.00% <100.00%> (ø)
neurokit2/ppg/ppg_findpeaks.py 98.14% <100.00%> (ø)
neurokit2/ppg/ppg_report.py 100.00% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Sep 14 '22 00:09 codecov-commenter

(I'll get to this asap, gotta finish some work first :)

DominiqueMakowski avatar Sep 21 '22 01:09 DominiqueMakowski

Finally found time to give this a look, and I must say it's awesome 😍

I renamed a bunch of stuff (I tend to over-rename things in general whenever I am reviewing code) and reoganized; mostly I separated ppg_report() creation per se from the ppg_methods() sanitation, put the report content wrangling stuff into misc/. But it's great I think it will be one of the biggest feature yet!

Before we start adapting it to the rest of the signals, let's make this one top notch, I think we need to:

  • Somehow test all the combinations to make sure there's no blindspot (not sure how to best test that though)
  • Add some style to the HTML file
    • References could be bullet points
    • Add a title to the report ("NeuroKit Processing Report" (?))
    • Add some style. This is beyond my abilities though, the best I managed to do recently is to add some CSS in the preamble of an HTML file (which renders like this). We shouldn't spend too much time on style for now though, and keep it miminal, but simply changing the font, having the sections with a background color could already nicely separate the sections
    • Print some of the text in the console as well

DominiqueMakowski avatar Sep 29 '22 01:09 DominiqueMakowski

Finally found time to give this a look, and I must say it's awesome 😍

🎉

I renamed a bunch of stuff (I tend to over-rename things in general whenever I am reviewing code) and reoganized; mostly I separated ppg_report() creation per se from the ppg_methods() sanitation, put the report content wrangling stuff into misc/.

Awesome, honestly this is a lot nicer than me working alone overthinking my own names/organization...

Before we start adapting it to the rest of the signals, let's make this one top notch, I think we need to:

* Somehow test all the combinations to make sure there's no blindspot (not sure how to best test that though)

Were you thinking automatic tests that just confirm that a file is generated without errors? Or more that we generate all examples and confirm that they work as expected?

* Add some style to the HTML file
  
  * References could be bullet points
  * Add a title to the report ("NeuroKit Processing Report" (?))
  * Add some style. This is beyond my abilities though, the best I managed to do recently is to add some CSS in the [preamble](https://github.com/RealityBending/IllusionGameValidation/blob/main/index.html#L3-L27) of an HTML file (which renders like [this](https://realitybending.github.io/IllusionGameValidation/)). We shouldn't spend too much time on style for now though, and keep it miminal, but simply changing the font, having the sections with a background color could already nicely separate the sections

Newest version is now here: https://danibene.github.io/neurokit2-ppg-report-example/myreport.html

  * Print some of the text in the console as well

This is what the corresponding text now looks like:

The raw signal, sampled at 500 Hz, was directly used for peak detection without preprocessing.

The peak detection was carried out using the method described in Elgendi et al. (2013).

Elgendi M, Norton I, Brearley M, Abbott D, Schuurmans D (2013) Systolic Peak Detection in Acceleration Photoplethysmograms Measured from Emergency Responders in Tropical Conditions. PLoS ONE 8(10): e76585. doi:10.1371/journal.pone.0076585.

|   PPG_Rate_Mean |   PPG_Rate_SD |
|----------------:|--------------:|
|         66.9511 |       19.2591 |

danibene avatar Sep 29 '22 13:09 danibene

Also, I'm wondering if we should save the text + references in a separate file like .json, .rst, .csv, .bib etc. rather than hard coding them?

E.g. https://github.com/danibene/cite-neurokit2-hrv/blob/main/neurokit2_hrv_feat_info.csv

Then it could be more easily edited by people who don't necessarily want to touch the code & reused for things like generating tables in LaTeX?

danibene avatar Sep 29 '22 14:09 danibene

rather than hard coding them?

Well it would still be hardcoded wouldn't it 😁 The problem of having non .py files in the packages it's that then we need to ship additional files etc. and it complicates a bit the distribution. It's feasible, but if we can keep python files it makes things easier. That being said, we could easily store the same info that you linked in a "hard-coded" python dict instead of a csv/json, there is not much difference (people could easily the contents of such file) afaik?

Though then in general we should also keep in mind that we cannot cater to everyone's need and avoid creating a maintenance beast for us: the report should be geared as a convenient way to get the info in a reproducible manner, but then if people want more control and customization and all, they might as well re-create it themselves to be how they want

Were you thinking automatic tests that just confirm that a file is generated without errors? Or more that we generate all examples and confirm that they work as expected?

I guess both, probably the first (that it doesn't error) is more important, then if there are some issues in the report's content I'm sure users will report them in issues.

DominiqueMakowski avatar Sep 30 '22 01:09 DominiqueMakowski

The problem of having non .py files in the packages it's that then we need to ship additional files etc. and it complicates a bit the distribution. It's feasible, but if we can keep python files it makes things easier.

That makes sense

I guess both, probably the first (that it doesn't error) is more important, then if there are some issues in the report's content I'm sure users will report them in issues.

I added a test, let me know if you think we should change it

danibene avatar Oct 01 '22 17:10 danibene

Looks very good to me, feel free to merge!

DominiqueMakowski avatar Oct 03 '22 02:10 DominiqueMakowski