adding basic set of visualizations #5

SRSteinkamp · 2023-07-21T16:07:09Z

Closes #

Adding the following plotting functions:

raw data plot (from peakdet)
average peak (using a peak detection step)
power spectrum
histogram wrapper

Proposed Changes

Adding visualization submodule to the new interface folder.

Change Type

Checklist before review

I added everything I wanted to add to this PR.
[Code or tests only] I wrote/updated the necessary docstrings.
[Code or tests only] I ran and passed tests locally.
[Documentation only] I built the docs locally.
My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
My code respects the adopted style, especially linting conventions.
The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
I added or indicated the right labels.

I added information regarding the timeline of completion for this PR.

Please, comment on my PR while it's a draft and give me feedback on the development!

m-miedema · 2024-04-18T19:32:41Z

physioqc/interfaces/visualizations.py

+    if phys.fs > 1_000:
+        #  This should probably go to the logs, or warning
+        print(f"Fs = {phys.fs} Hz too high, resampling to {target_fs} Hz")
+        phys = pk.operations.interpolate_physio(phys, target_fs, kind="linear")


Could consider raising an error if target_fs > 1000 Hz.

As with the sentiment above, I think it should be possible to re-sample to larger than 1000 Hz if one really wants to and know what they are doing.

The warning / message is probably misleading. I think I would change it to phys.fs > target_fs and display the message: "Fs = xx Hz higher than target_fs, resampling to target_fs or something.

For plotting purposes, I still think it makes sense to enforce an upper limit for the sampling rate - and 1000 Hz seemed reasonable enough for me, at the Brainhack ...

m-miedema

In general, the changes are functional and look good! I was testing using Python 3.11.4 in Windows. I am leaving some specific feedback to improve small things throughout, particularly for the plot_average_peak function.

Other things that should be considered, possibly in a future PR (?) but also reasonably in this one, include:

functionality to save plots
logging functionality
raising errors, especially if the plotting function does not receive a Physio-type object
adding test coverage

physioqc/interfaces/visualizations.py

m-miedema · 2024-04-18T19:48:14Z

physioqc/interfaces/visualizations.py

+    phys: pk.Physio,
+    window: List = [-3, 3],
+    target_fs: float = 1000.0,
+    peak_dist: float = 1.0,
+    peak_thr: float = 0.1,


In my opinion, a better way to implement this function would be to check if the Physio object already has undergone peak detection, and if so, to use the existing peaks.

Right now, it seems like these peak detection variables are buried in a fairly unexpected place. It could improve functionality in the long run to add these as fields in the Physio object itself, since they seem to be frequently specified throughout this package and it is important to be consistent with them if they need to be altered for different data.

I think one decision we made, is that peak-detection is fast enough, so we do it here specifically for plotting purposes, on a potentially lower sampling rate than required for other estimates of the signal.

Did you test whether these parameters robustly detected peaks across tricky data and a number of physio-types? My concern is that the way they are specified in the function here may not translate to different data types (e.g. a relatively high heart rate, or data with an unexpected scaling or low SNR). Since these parameters will end up being embedded (maybe unexpectedly to the end user) inside the function call, they might not be obvious to change. If the user has spent some time optimizing peak detection with peak det, it makes sense to propagate those parameters forward here.

Didn't really test this (don't really have a good grasp on the parameters). Just remembered, the reason for redoing peak detection was due to this bug https://github.com/physiopy/peakdet/issues/63, and higher fs than 1000Hz is too much for plotting. We've also been operating under the idea of applying QC to rawdata first, we did not want to pre-suppose a fixed pipeline.
Until the bug is fixed, I'd suggest, to leave the function as is. But for the automated pipeline, I think we should have an autodetection of modality and set some default parameters accordingly, but also allow the parameters to be overridden by the user in the CLI.

m-miedema · 2024-04-18T19:49:35Z

physioqc/interfaces/visualizations.py

+    peaks = list(
+        filter(
+            lambda ps: ((ps + window[0]) >= 0)
+            and ((ps + window[1] + 1) < phys.data.shape[0]),
+            phys.peaks,
+        )
+    )


This part was a bit confusing to me, why are we not using these peaks?

If I remember correctly, it is discarding peaks where the data is not fully covered by the window. The idea of the figure is to plot the signal shape around peaks. (Hope that makes sense)

That does, thanks! That's my understanding of what it's doing, but I'm wondering whether it wouldn't make more sense to include all peaks in the average, regardless of whether some data is being replicated inside the window.

Not sure what you mean with "replicated". The filtering makes the process slightly easier, as it avoid indexing errors. Otherwise, some padding would be needed, but that complicates the timing (very slightly).

Oh, I see, I misunderstood. That makes sense to me the way it is then, thanks! Maybe add a comment to say that this is just a question of indexing errors so some peaks at the beginning/end of the series may be excluded.

m-miedema · 2024-04-18T19:52:57Z

physioqc/interfaces/visualizations.py

+    freqs, psd = multimodal.power_spectrum(phys)
+
+    ax.plot(freqs, psd)
+    ax.set(xlabel="Frequencies", ylabel="V^2/Hz")


Better to be more specific with the units we do know (Frequencies (Hz)) and not assume volts (Power Spectral Density)

Units are really not my thing (: ... what would that be for power a power spectrum? P/Hz?

For the PSD, the units depend on the units of your time series. Since this isn't something we ask for (e.g. could be mV, could be V, could be something else entirely) it's better just not to include it.

Alternatively, we could have a default unit (e.g. [a.u.]), except for when they are specified by the user (e.g. in BIDS metadata or in the CLI), in which case we can be more specific

physioqc/interfaces/visualizations.py

beccaclements99

This looks great! I was testing using Python 3.11.6 on a Mac. I agree with Mary's comments, and also left some comments suggesting minor changes to the function descriptions to improve clarity. I also suggested some non-essential changes to the plot_histogram function

physioqc/interfaces/visualizations.py

beccaclements99 · 2024-04-18T20:47:38Z

physioqc/interfaces/visualizations.py

+
+    fig, ax = check_create_figure(ax, figsize=(7, 5))
+
+    ax.hist(signal)


It isn't essential, but you could consider adding axis labels for the histogram and the option for the user to change the number of bins

I agree with adding bins, but not sure if labels should be added here, as it really is just supposed to be a wrapper for the histogram function.

beccaclements99 · 2024-04-18T21:13:06Z

physioqc/interfaces/visualizations.py

+    window : List, optional
+        window size around the peak in s, by default [-3, 3]
+    target_fs : float, optional
+        sampling rate for plotting and peak detection, by default 1000.0


It seems like target_fs is only used if the sampling rate of the data is greater than 1000, so it'd be helpful to mention that here

SRSteinkamp · 2024-04-19T07:30:44Z

In general, the changes are functional and look good! I was testing using Python 3.11.4 in Windows. I am leaving some specific feedback to improve small things throughout, particularly for the plot_average_peak function.

Other things that should be considered, possibly in a future PR (?) but also reasonably in this one, include:
* [ ]  functionality to save plots

* [ ]  logging functionality

* [ ]  raising errors, especially if the plotting function does not receive a Physio-type object

* [ ]  adding test coverage

Hi, thanks for the feedback, the idea for PhysioQC (as far as I remember) was to create MRIQC like reportlets. I have that already implemented in a very quick and dirty fashion, building on physiopy/peakdet#11

In principle, what would happen is that you call the CLI on the raw physiological data in your BIDS directory and it creates html outputs. In that workflow the images would be saved.

That logic might provide some context regarding a few of the coding decisions. But also it has been a long while, so I'll have a closer look :)

Do you have a good guideline for implementing logging? That is something I have no experience in.

smoia · 2024-04-19T09:54:24Z

I'm not sure this is exactly what you are looking for, but:
https://betterstack.com/community/guides/logging/loguru/

(see also phys2bids for a standard python logger implementation: https://github.com/physiopy/phys2bids/blob/d796fe17f7af4aed9605bd51f4ab3d8c0609a3ae/phys2bids/phys2bids.py#L46)

In general, you will need to declare a log object at the beginning of each file (always the same object), then any time you would print something, make a log item instead (e.g. here).
If the print message is utterly important (e.g. you're changing an input parameter, or something doesn't seem right with the input parameters, but not wrong enough to raise an exception, or you're doing some unexpected trick to make the input work, e.g. transposing arrays) that's a log warning. If the print message is to inform the user that something is happening (good to do that from time to time), that's a log info, and if you are implementing loguru, it's good to throw a success every now and then after something worked out (e.g. you finished computing a metric, or created a graph, or put together the report).
@maestroque might be able to help you better!

Then, in the workflow & CLI, we can make implement a logger report level (like here)

m-miedema · 2024-04-19T13:27:45Z

For logging specifically, I think we can make another PR - what do you think @smoia ? It's mostly the code coverage/testing that I think needs to be added in before we can merge this. But I'm not sure of the current approach to tests in this toolbox, so let me know if that's also something better to address later.

smoia · 2024-04-19T14:25:38Z

That sounds like a sensible idea - especially after physiopy/peakdet#11 is merged as well, so we have one complete log-related PR. It would be better to open an issue to track the log points raised here though!

SRSteinkamp · 2024-04-19T14:28:23Z

I know this might break the general development cycle, but it might be worth also move testing to a different PR so we can make progress on physiopy/peakdet#11 .

m-miedema · 2024-04-19T14:47:47Z

@SRSteinkamp I agree! In that case, I think that the main thing to address before we merge the PR are the specific comments we've made so far. @smoia what are your thoughts about opening a separate issue for adding test coverage?

m-miedema · 2024-04-19T14:48:55Z

Actually, there's already an open Loguru issue, so no need to open a new one for logging.

smoia · 2024-04-19T15:00:50Z

We've never been strict about testing, so I'm inclined to say ok, given we're not in beta stage yet - as long as the testing happens (chatGPT is a great friend for that), that's fine!

m-miedema

I think we're good to go on this for now and move over to the workflow PR, yes? :)

smoia · 2024-06-20T03:30:07Z

🚀 PR was released in 0.4.0 🚀

SRSteinkamp added 10 commits July 21, 2023 15:38

plot for raw data

8b33083

plot for average peak

db12cd1

plot for power spectrum

467f87e

corrected type annotation in peak_detection

a3f36e6

update visualization for peak amplitude

51055b1

renaming axes to ax

001fa44

changed visualization to also can use provided figure axis

82c20f9

added histogram wrapper

a878f06

Typing does not like lists as output - they are tuples now

ec5e263

updated doc for plot_histogram

a433aa7

SRSteinkamp added Enhancement New feature or request Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) and removed Enhancement New feature or request labels Dec 4, 2023

SRSteinkamp mentioned this pull request Dec 5, 2023

Implementation of the workflow #11

Merged

8 tasks

SRSteinkamp requested a review from m-miedema December 6, 2023 08:53

SRSteinkamp assigned SRSteinkamp and unassigned SRSteinkamp Feb 16, 2024

smoia assigned SRSteinkamp and m-miedema Apr 18, 2024

m-miedema reviewed Apr 18, 2024

View reviewed changes

m-miedema requested changes Apr 18, 2024

View reviewed changes

m-miedema reviewed Apr 18, 2024

View reviewed changes

physioqc/interfaces/visualizations.py Outdated Show resolved Hide resolved

m-miedema reviewed Apr 18, 2024

View reviewed changes

physioqc/interfaces/visualizations.py Outdated Show resolved Hide resolved

beccaclements99 suggested changes Apr 18, 2024

View reviewed changes

SRSteinkamp added 2 commits April 19, 2024 16:52

updated documentation

9d9ffc6

added bins to histogram wrapper

3fe83fb

smoia mentioned this pull request Jul 25, 2024

Interpolation of physio objects after peak detection does not interpolate peaks/troughs physiopy/prep4phys#3

Open

SRSteinkamp added 2 commits April 22, 2024 09:35

changing label to a.u.

5fba98a

updated comments and message in aerage peak plots

6b7eeb2

m-miedema approved these changes May 8, 2024

View reviewed changes

SRSteinkamp merged commit 0855a9c into physiopy:master Jun 20, 2024
1 check passed

smoia added the released This issue/pull request has been released label Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding basic set of visualizations #5

adding basic set of visualizations #5

SRSteinkamp commented Jul 21, 2023 •

edited by smoia

Loading

m-miedema Apr 18, 2024

SRSteinkamp Apr 19, 2024

m-miedema left a comment

m-miedema Apr 18, 2024

SRSteinkamp Apr 19, 2024 •

edited

Loading

m-miedema Apr 19, 2024 •

edited

Loading

SRSteinkamp Apr 22, 2024

m-miedema Apr 18, 2024

SRSteinkamp Apr 19, 2024

m-miedema Apr 19, 2024

SRSteinkamp Apr 19, 2024

m-miedema Apr 19, 2024

m-miedema Apr 18, 2024

SRSteinkamp Apr 19, 2024

m-miedema Apr 19, 2024

smoia Apr 19, 2024

beccaclements99 left a comment

beccaclements99 Apr 18, 2024

SRSteinkamp Apr 19, 2024

beccaclements99 Apr 18, 2024

SRSteinkamp commented Apr 19, 2024

smoia commented Apr 19, 2024 •

edited

Loading

m-miedema commented Apr 19, 2024 •

edited

Loading

smoia commented Apr 19, 2024

SRSteinkamp commented Apr 19, 2024

m-miedema commented Apr 19, 2024

m-miedema commented Apr 19, 2024

smoia commented Apr 19, 2024

m-miedema left a comment

smoia commented Jun 20, 2024


		fig, ax = check_create_figure(ax, figsize=(7, 5))

		ax.hist(signal)

adding basic set of visualizations #5

adding basic set of visualizations #5

Conversation

SRSteinkamp commented Jul 21, 2023 • edited by smoia Loading

Proposed Changes

Change Type

Checklist before review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-miedema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SRSteinkamp Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

m-miedema Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beccaclements99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SRSteinkamp commented Apr 19, 2024

smoia commented Apr 19, 2024 • edited Loading

m-miedema commented Apr 19, 2024 • edited Loading

smoia commented Apr 19, 2024

SRSteinkamp commented Apr 19, 2024

m-miedema commented Apr 19, 2024

m-miedema commented Apr 19, 2024

smoia commented Apr 19, 2024

m-miedema left a comment

Choose a reason for hiding this comment

smoia commented Jun 20, 2024

SRSteinkamp commented Jul 21, 2023 •

edited by smoia

Loading

SRSteinkamp Apr 19, 2024 •

edited

Loading

m-miedema Apr 19, 2024 •

edited

Loading

smoia commented Apr 19, 2024 •

edited

Loading

m-miedema commented Apr 19, 2024 •

edited

Loading