Skip to content
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

[Fix] ppg_plot() was not returning matplotlib.Figure #678

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

luisqtr
Copy link
Contributor

@luisqtr luisqtr commented Jul 15, 2022

Hi...

Description

I needed to store the results of ppg_plot() in a file on disk (easier to inspect than doing it manually in the notebook). However, the function did not return the object matplotlib.figure so I could use fig.savefig([path]), even though the documentation says that it returns a figure.

If the PR is accepted, I can check similar behavior for the other variables.

Proposed Changes

  • Return the object matplotlib.figure.Figure for ppg_plot() instead of just plotting the image on the notebook.
  • [Docs] Minor edit: Added description about feature LFHF returned by hrv_frequency(), it was missing.

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • I have read the CONTRIBUTING file.
  • My PR is targetted at the dev branch (and not towards the master branch).
  • 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)

@welcome
Copy link

welcome bot commented Jul 15, 2022

Thanks for opening this pull request! We'll make sure it's perfect before merging 🤗 force
Make sure to read the contributing guide. Also, if you think that your contribution is worthy of it, you can consider adding yourself to the Contributors list (feel free to ask us if you have any doubts).

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022

Codecov Report

Merging #678 (16311e9) into dev (0bc49ca) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev     #678      +/-   ##
==========================================
- Coverage   52.42%   52.38%   -0.04%     
==========================================
  Files         276      276              
  Lines       12424    12430       +6     
==========================================
- Hits         6513     6512       -1     
- Misses       5911     5918       +7     
Impacted Files Coverage Δ
neurokit2/ecg/ecg_plot.py 90.47% <ø> (ø)
neurokit2/emg/emg_plot.py 97.77% <ø> (ø)
neurokit2/eog/eog_plot.py 92.15% <ø> (ø)
neurokit2/hrv/hrv_frequency.py 61.70% <ø> (ø)
neurokit2/ppg/ppg_plot.py 13.79% <ø> (ø)
neurokit2/rsp/rsp_plot.py 93.87% <ø> (ø)
neurokit2/signal/signal_plot.py 66.66% <ø> (ø)
neurokit2/rsp/rsp_simulate.py 93.75% <0.00%> (-4.47%) ⬇️
neurokit2/signal/signal_detrend.py 37.70% <0.00%> (-0.48%) ⬇️
neurokit2/signal/signal_filter.py 64.89% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bc49ca...16311e9. Read the comment docs.

@DominiqueMakowski
Copy link
Member

Thanks for that PR, unfortunately, we recently changed that behaviour on behaviour to not return the figure, as it was causing some issues, among which double figures showing up in the console and inconsistent return type which is not a good programming practice (that said, it's possible that documentation has to be adjusted in some places!)

In principle thanks to the arcanes of matplotlib you can still recover the figure and save it to disk as follows:

import matplotlib.pyplot as plt
import neurokit2 as nk

# Generate 15 seconds of PPG signal (recorded at 250 samples / second)
ppg = nk.ppg_simulate(duration=15, sampling_rate=250, heart_rate=70, random_state=333)

# Process it
signals, info = nk.ppg_process(ppg, sampling_rate=250)

# Visualize the processing
nk.ppg_plot(signals, sampling_rate=250)

# Save it
plt.tight_layout()
plt.savefig("ppg.png", dpi=300)

Or even

fig = plt.gcf()
fig.set_size_inches(10, 6, forward=True)
fig.savefig("ppg2.png", dpi=300)

Let us know if it works, and if it does, you can edit your PR to just leave the documentation fix :)

@luisqtr
Copy link
Contributor Author

luisqtr commented Jul 18, 2022

Thanks for your feedback 😁. Your example code to save the plots worked very well!
I have updated the docs related with plot functions, deleting the Return and adding an example to save figures to PNG files.

@luisqtr
Copy link
Contributor Author

luisqtr commented Jul 18, 2022

Apparently there is an error building the new docs induced by my change in ecg_plot(). This is the first time doing a PR and working with sphinx, so I am not completely sure what went wrong 😅. How can I reproduce locally the Documentation check before doing a new commit?

@DominiqueMakowski
Copy link
Member

I fixed the issues, merging now :) thanks!

@DominiqueMakowski DominiqueMakowski merged commit 7ad0c51 into neuropsychology:dev Jul 22, 2022
@welcome
Copy link

welcome bot commented Jul 22, 2022

landing
Congrats on merging your first pull request! 🎉🍾 We're looking forward to your next one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants