-
-
Notifications
You must be signed in to change notification settings - Fork 413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Report describing methods in PPG process #707
[Feature] Report describing methods in PPG process #707
Conversation
…ene/NeuroKit into feature/ppg_process_report
Codecov ReportBase: 52.74% // Head: 53.16% // Increases project coverage by
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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
(I'll get to this asap, gotta finish some work first :) |
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 Before we start adapting it to the rest of the signals, let's make this one top notch, I think we need to:
|
🎉
Awesome, honestly this is a lot nicer than me working alone overthinking my own names/organization...
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?
Newest version is now here: https://danibene.github.io/neurokit2-ppg-report-example/myreport.html
This is what the corresponding text now looks like:
|
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? |
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
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. |
That makes sense
I added a test, let me know if you think we should change it |
Looks very good to me, feel free to merge! |
See #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 argumentreport
.(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