Integrate physutils - Physio object usage by maestroque · Pull Request #54 · physiopy/phys2denoise

maestroque · 2024-06-12T23:31:48Z

Proposed Changes

Integrate physutils (currently only testing locally as the package is pending publishing)
Integrate the use of the Physio object for the metric functions
Retain the ability for the functions to be used without the need for the Physio object

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!

…unctions

maestroque · 2024-06-12T23:35:36Z

Currently tests are failing, because the physutils module cannot be installed

m-miedema

The main thing that I think we need to discuss is checking for peaks/troughs where appropriate in the Physio object. I'm not sure the best way to do this (and we've talked about maintaining functionality for non-Physio object time-series, so we'll need to think about that in this context too). Many parameters e.g. TR could equally be associated with a Physio object, so we need a clear plan that checks for parameters not supplied explicitly but allows flexibility for a non-Physio implementation.

m-miedema · 2024-06-17T23:29:15Z

phys2denoise/metrics/multimodal.py

-    sample_rate : float
-        Physio sample rate, in Hertz.
+    data : Physio_like
+        Object containing the timeseries of the recorded respiratory or cardiac signal


I would suggest including a bit more information here, especially that the object should also contain associated meta-data.

(I think it's worth a discussion where we talk about advantages of forcing some parameters to be in the Physio object vs. explicitly specified before we go too far down this road!)

maestroque · 2024-06-24T16:07:34Z

I believe the support for Physio and non-Physio inputs has been completed. @m-miedema @me-pic let me know your thoughts

The unit tests now need updating.

maestroque · 2024-06-24T19:19:54Z

In CRF, iCRF and RRF there is this line:

time_stamps = np.arange(0, time_length, 1 / samplerate)

Since samplerate is in seconds, shouldn't it be

time_stamps = np.arange(0, time_length, samplerate)

Do you prefer changing the doc to Hz or is it more convinient in seconds?
@m-miedema @me-pic

maestroque · 2024-06-24T19:27:23Z

Also in general, shouldn't we opt in a single way to define sample rate? Is there any reason to not have everything in Hz?

…tory

…ry with each operation

maestroque · 2024-06-24T21:00:54Z

phys2denoise/tests/test_metrics_chest_belt.py

    assert rrf_arr.size == pred_len


-@mark.xfail


Not sure why this was marked as xfail, but now it's fixed and it passes.

That's also mentioned in #41

…orpad_exception

maestroque · 2024-07-24T12:07:35Z

@m-miedema metrics calculations are already integrated with the history. This new attribute is to store all the metric values apart from only the history. This attribute can be also used in physioqc for example

maestroque · 2024-08-10T16:12:46Z

Currently blocked by physiopy/physutils#4, due to important physio object fix (release needed): 0c324c5 and 2b0deb9

maestroque · 2024-08-10T17:47:24Z

@smoia @me-pic @m-miedema I added a wrapper to sort out the returns (only metric or physio with metric), feel free to review.
The failing tests currently are due to:

physutils package not being up to date with Add Physio object generation from BIDS physutils#4, where I pushed some bug fixes
Existing fail in master due to a failed error assertion. The error is indeed raised but the assertion does not catch it.

Also, the following changes were made to deps:

numpy: >=1.9.3, <2
matplotlib: non-constrained

phys2denoise/tests/test_metrics_cardiac.py

maestroque · 2024-08-11T23:20:28Z

I believe the Metric class will make it much easier for the pydra tasks to handle computing and exporting metrics

.all-contributorsrc

smoia · 2024-08-12T11:20:20Z

setup.cfg

 install_requires =
-    numpy >=1.9.3
-    matplotlib >=3.1.1
+    numpy >=1.9.3, <2


Can you open an issue to state we need to refactor code to handle numpy >=2.0, please?

(It's good to leave it like this for the moment, though, thanks!)

Opened here: #62

smoia · 2024-08-12T11:21:22Z

setup.cfg

    pytest >=5.3
    pytest-cov
-    peakdet
+    peakdet>=0.5.0


With the physutils integration, do we still need peakdet?

yes, there is a unit test with peak finding

smoia · 2024-08-12T11:25:01Z

phys2denoise/metrics/utils.py

+    """
+
+    def determine_return_type(func):
+        metrics_with_lags = ["respiratory_variance_time"]


We did not implemented it yet, but technically all metrics could have lags - and all metrics could be convolved. Is this a way to check the type of output you get?

Could you instead make a check on the provided arguments to the function, to check whether convolution and/or lag happened?

I'll change to detect lagged metrics, since it's dependent on the argument. However convolution is now only done in some metrics without being specified in the arguments and can't be detected easily if you don't read the code. Also you can't deduce that the metric is convolved from the dimensions of the metric output.

I can open an issue about it if you want

An issue sounds good to me, you're right that we can improve transparency and flexibility for those operations as we build up the toolbox. If you don't have time I can open one, let me know your preference @maestroque :)

smoia · 2024-08-12T11:31:01Z

phys2denoise/metrics/chest_belt.py

       deep breaths," Neuroimage, vol. 204, 2020.
    """
+
+    def _respiratory_pattern_variability(data, window):


Now that you can either input a physio object or a numpy array, do we still need to duplicate code for this application?

That's true the relevant line in env() could be changed as such:

env_arr = ( pd.Series(data.data) .rolling(window=window, center=True) .apply(respiratory_pattern_variability, args=(window,), kwargs=dict(return_physio=False)) )

However we should also include support for pandas.core.series.Series types in the load_physio function for that to work, which also impacts the other module. Still, I think it'd be better to open an issue about it, because it complicates the PRs
Wdyty @smoia

smoia · 2024-08-12T11:31:51Z

phys2denoise/metrics/chest_belt.py

+@return_physio_or_metric()
+@physio.make_operation()
+def respiratory_variance_time(
+    data, fs=None, peaks=None, troughs=None, lags=(0, 4, 8, 12), **kwargs


Suggested change

data, fs=None, peaks=None, troughs=None, lags=(0, 4, 8, 12), **kwargs

data, peaks=None, troughs=None, fs=None, lags=(0, 4, 8, 12), **kwargs

It would be good to have peaks and troughs first when possible (and, in general, avoid changing the order of inputs as much as possible), but if it makes things too complicated with the wrapper, then no problem.

smoia

Couple of notes for the moment!

smoia · 2024-08-12T11:33:17Z

phys2denoise/metrics/chest_belt.py

+@return_physio_or_metric()
+@physio.make_operation()
+def respiratory_variance_time(
+    data, fs=None, peaks=None, troughs=None, lags=(0, 4, 8, 12), **kwargs


It would be good to have peaks and troughs first when possible (and, in general, avoid changing the order of inputs as much as possible), but if it makes things too complicated with the wrapper, then no problem.

smoia · 2024-08-12T11:43:36Z

phys2denoise/metrics/utils.py

+    """
+
+    def determine_return_type(func):
+        metrics_with_lags = ["respiratory_variance_time"]


Could you instead make a check on the provided arguments to the function, to check whether convolution and/or lag happened?

smoia · 2024-08-12T11:48:37Z

phys2denoise/metrics/chest_belt.py

-        troughs found by peakdet algorithm
-    samplerate: float
-        sample rate in hz of respiratory belt
+    data : Physio_like


Could we change this to physutils.Physio, np.ndarray, or array-like object instead?
That's also possible now, right?

smoia · 2024-08-12T11:51:32Z

phys2denoise/metrics/chest_belt.py

       fluctuations in fMRI”, NeuroImage, vol.31, pp. 1536-1548, 2006.
    """
-    timestep = 1 / samplerate
+    if isinstance(data, physio.Physio):


I was that you repeat this few following lines often in different metrics - if that is the case, I would suggest to make it a function on its own or even better to add it to the decorator you created. If you make it a function, the return can be done within the metric function itself (without the wrapper), if you move this to the decorator (better), you can revert to the original metric functions, and deal with the input and returns of the metrics within the wrapper!

I tried to think of a cleaner way, but tbh the error handling cannot be abstracted efficiently, because there are some differences between functions using fs, peaks, troughs and combinations of them. Effectively, if an abstraction would be made it would almost have to be a separate handle case for each function. It could be done by parsing the argument name list in the wrapper but I don't think it can be much cleaner than what exists

maestroque · 2024-08-21T15:48:21Z

@smoia please recheck when you find some time

m-miedema

With the updated physutils this looks good to me! I don't see a reason not to fix the one test I mentioned on the way out but (feedback from @smoia pending) I would be happy to see this move on and into the workflow.

m-miedema · 2024-08-21T22:00:32Z

phys2denoise/tests/test_metrics_utils.py

    function should give an error."""
    arr = np.array(short_arr)
-    with pytest.raises(Exception) as e_info:
+    with pytest.raises(IndexError):


I get the expected logger warning here, so the test fails. I'm guessing this one is what you were referring to as the failed catch here? If so, I'm wondering what your suggestion is for handling it to catch the warning instead.

Yes that's the one. It's not detected because it's being handled. I'll just add a check that the subsequent warning is raised

It's a bit convoluted though and it's pending a full integration of loguru in this package as well. I'd say we merge and document in the issue/PR #53

I did a quick fix and it detects the expected warning text (as in the peakdet warning assertion tests I did), however the full integration of loguru needs to be adressed: Relevant note

@m-miedema

Thanks @maestroque !

codecov · 2024-08-25T13:39:48Z

Codecov Report

Attention: Patch coverage is 73.26733% with 54 lines in your changes missing coverage. Please review.

Project coverage is 53.85%. Comparing base (24515ca) to head (6266b6f).
Report is 149 commits behind head on master.

Files	Patch %	Lines
phys2denoise/metrics/cardiac.py	53.33%	21 Missing ⚠️
phys2denoise/metrics/multimodal.py	17.39%	19 Missing ⚠️
phys2denoise/metrics/chest_belt.py	87.87%	8 Missing ⚠️
phys2denoise/metrics/utils.py	90.47%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   47.04%   53.85%   +6.81%     
==========================================
  Files           7        8       +1     
  Lines         321      596     +275     
==========================================
+ Hits          151      321     +170     
- Misses        170      275     +105

Files	Coverage Δ
phys2denoise/metrics/responses.py	`94.44% <100.00%> (ø)`
phys2denoise/metrics/utils.py	`67.23% <90.47%> (-2.34%)`	⬇️
phys2denoise/metrics/chest_belt.py	`92.38% <87.87%> (+6.83%)`	⬆️
phys2denoise/metrics/multimodal.py	`26.82% <17.39%> (-6.51%)`	⬇️
phys2denoise/metrics/cardiac.py	`54.34% <53.33%> (-40.39%)`	⬇️

... and 2 files with indirect coverage changes

maestroque · 2024-08-25T13:40:40Z

Now, CI passes online as well, because of the physutils release. I'm merging @m-miedema @smoia

maestroque added 6 commits June 12, 2024 18:53

Add physutils dependency placeholder

2536086

Initial Physio object integration in cardiac.py

f047587

Style fixes

c7fc2eb

Minor cardiac.py physio initialization fix

ec8a3de

Integrate Physio object for respiratory and multimodal metrics

67c4d21

Add sample support for object and function oriented usage of metric f…

2ffa465

…unctions

maestroque requested review from m-miedema, me-pic and smoia June 12, 2024 23:31

maestroque self-assigned this Jun 12, 2024

maestroque changed the title ~~Integrate physutils - Physio object usage~~ [WIP] Integrate physutils - Physio object usage Jun 12, 2024

m-miedema requested changes Jun 17, 2024

View reviewed changes

maestroque added 4 commits June 24, 2024 16:08

Make metric functions operations

9fa3467

Add Physio and non-Physio compatibility to cardiac.py

380ab65

Add Physio and non-Physio compatibility to chest_belt.py

59ee6c8

Add Physio and non-Physio compatibility to retroicor

56abb72

maestroque mentioned this pull request Jun 24, 2024

Add auto-author and auto-label to github workflow #52

Merged

18 tasks

Fix CRF, iCRF and RRF calculation

2c19a65

maestroque added 4 commits June 24, 2024 23:09

Fix cardiac_phase test, return Physio object and metric to update his…

c28914c

…tory

Fix chest_belt tests, return Physio object and metric to update histo…

625d45f

…ry with each operation

Update RVT test

acf960d

Fix respiratory_phase_smoke test

62e4b6f

maestroque commented Jun 24, 2024

View reviewed changes

maestroque added 2 commits June 25, 2024 00:07

Return physio in retroicor and specify raised error type in test_mirr…

e60d49e

…orpad_exception

Add loguru as a dependency

3dbf656

maestroque mentioned this pull request Jun 25, 2024

Test in metrics.chest_belt fails, possibly due to deprecation #41

Open

Merge branch 'physiopy:master' into integrate-physutils

00b0f56

maestroque added 2 commits August 10, 2024 20:40

Add wrapper to determine whether to return a metric or a physio object

bc300f0

[deps] Cap numpy to <2.0

9883ce2

maestroque added 2 commits August 10, 2024 20:51

[docs]: Update computing metrics

181304f

Metric or Physio returning optimization

43107db

maestroque commented Aug 11, 2024

View reviewed changes

phys2denoise/tests/test_metrics_cardiac.py Outdated Show resolved Hide resolved

Add Metric class for easier properties accessing

b0f4110

smoia reviewed Aug 12, 2024

View reviewed changes

.all-contributorsrc Show resolved Hide resolved

smoia reviewed Aug 12, 2024

View reviewed changes

maestroque added 2 commits August 21, 2024 17:13

More unit tests, physio/metric wrapper optimization, doc fixes

10509bd

Minor fix

612c616

m-miedema approved these changes Aug 21, 2024

View reviewed changes

[test] Fix failing warning assertion

e528d21

maestroque removed the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Aug 25, 2024

github-actions bot added the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Aug 25, 2024

Add updated physutils version

6266b6f

maestroque removed the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Aug 25, 2024

maestroque merged commit 087bd4e into physiopy:master Aug 25, 2024

	data, fs=None, peaks=None, troughs=None, lags=(0, 4, 8, 12), **kwargs
	data, peaks=None, troughs=None, fs=None, lags=(0, 4, 8, 12), **kwargs

Conversation

maestroque commented Jun 12, 2024 • edited by m-miedema Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Change Type

Checklist before review

Uh oh!

maestroque commented Jun 12, 2024

Uh oh!

m-miedema left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maestroque commented Jun 24, 2024

Uh oh!

maestroque commented Jun 24, 2024

Uh oh!

maestroque commented Jun 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maestroque commented Jul 24, 2024

Uh oh!

maestroque commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maestroque commented Aug 10, 2024

Uh oh!

Uh oh!

maestroque commented Aug 11, 2024

Uh oh!

Uh oh!

smoia Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-miedema Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maestroque commented Jun 12, 2024 •

edited by m-miedema

Loading

maestroque commented Aug 10, 2024 •

edited

Loading

smoia Aug 12, 2024 •

edited

Loading

m-miedema Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 25, 2024 •

edited

Loading