Migrate Physio class from peakdet #2

maestroque · 2024-06-05T19:07:04Z

Migrating the Physio class from peakdet to physutils, aiming to be used in the general physiopy toolset

For starters, this PR only aims to provide a Physio class to be used by peakdet and phys2denoise.

Changes

Add pre-commit hooks
Transfer Physio object
Also transfer
- plot_physio as a Physio method
- peakdet.io functions
- check_physio and new_physio_like functions as utils (TBC)

I will keep this description updated for further changes

me-pic · 2024-06-13T14:01:27Z

@maestroque Good job on that PR !
You will probably notice /have probably already notice, but the current tests are failing because of a phys2bids import in test_physio_obj.py. However phys2bids is not included as a installation requirement

maestroque · 2024-06-13T14:05:20Z

@me-pic I was thinking to remove this file (physio_obj.py) for the time being. Since it was only here as a placeholder from what @smoia had mentioned. In the end, the integration of physutils for phys2bids is not something we are currently interested in for the moment
What do you think?

maestroque · 2024-06-17T08:53:56Z

I deleted it for the time being, I think #1 covers us for the phys2bids object addition in the future, after the basic pipeline is down. Some features I'll add to the Physio object for the time being will be:

Labels
Physio object type
Option to generate dummy Physio objects from peaks

Then for the phys2bids object integration, we'll need to see if it can be done with a list of Physio objects. However, we'll need to figure out

If they will have a common history file apart from the individual history, and how that could play out
How signals like TR will be treated, which do not need to be Physio objects

m-miedema · 2024-06-17T23:18:03Z

I deleted it for the time being, I think #1 covers us for the phys2bids object addition in the future, after the basic pipeline is down. Some features I'll add to the Physio object for the time being will be:

Labels

Physio object type

Option to generate dummy Physio objects from peaks

Then for the phys2bids object integration, we'll need to see if it can be done with a list of Physio objects. However, we'll need to figure out

If they will have a common history file apart from the individual history, and how that could play out

How signals like TR will be treated, which do not need to be Physio objects

Sounds great! The labels will be to add BIDS complementarity i.e. subject and session info, if I'm interpreting this correctly from what we discussed earlier.

me-pic

Nice work so far on that PR @maestroque ! I've left some comments. Let me know if you want to discuss them !

me-pic · 2024-06-17T23:40:13Z

setup.cfg

@@ -22,7 +22,10 @@ provides =
 [options]
 python_requires = >=3.6.1
 install_requires =
+    matplotlib >=3.1.1, <=3.6.3, !=3.3.0rc1


Would there be a reason not to specify the latest version of matplotlib only (i.e., 3.9.0) ? I got an error trying to run pytest initially with matplotlib version 3.6.3: ImportError: numpy.core.multiarray failed to import. When I updated matplotlib to 3.9.0, all the imports were good

For this specific library I wouldn't think that there would be an issue updating, since the use of matplotlib is limited (only plot_physio() which is just an optional utility)

However, these limitations where posed from peakdet. I'm not sure if there was any specific reasoning or if there were any conflicts with the manual inspection GUI, maybe @smoia has some info on that

I spent some time on this because I have the same error trying to import matplotlib.pyplot in my virtual environment in the terminal (version 3.6.3). However, I added a quick test calling plot_physio() and explicitly importing matplotlib.pyplot which passed when I ran it in pytest!

`def test_import():

import matplotlib.pyplot as plt import matplotlib as matplotlib_pkg var = matplotlib_pkg.__version__ assert var == "3.6.3" phys = Physio(DATA, fs=1000) ax_phys = phys.plot_physio() assert plt.gcf().number == 1`

So, I'm not too sure what is going on here. If anyone has any insight I'd appreciate it, otherwise I guess we can proceed while keeping an eye out for any future version issues.

Side note: I would be interested to discuss how we add robust test coverage for plots!
@maestroque do you have any experience with this?

I'm not sure what kind of unit tests for plots you have in mind. For the time being the plotting function here does not have something convoluted (as in peakdet) other than just a plt.plot() call.

Also, again, I don't think there would be an issue updating the matplotlib module, these version constraints were just posed from peakdet. If there is any insight for this then ok, otherwise we can also proceed with 3.9 I don't think there would be an issue. Otherwise if there is indeed a reason for the version constraints in peakdet, we should keep them here because most probably someone would use a common environment for both modules physutils and peakdet

me-pic · 2024-06-18T00:09:28Z

physutils/io.py

+        logger.warning(
+            "History of provided Physio object is empty. Saving "
+            "anyway, but reloading this file will result in an "
+            "error."


Loading an empty history file does not lead to an error, but just to None. Maybe we can just rephrase the warning message here, so that it is more clear ?

Would it be more useful to throw an error when loading an empty history instead?

Are there any instances where we would want to load an empty history? Off-hand, I think a warning is preferable for flexibility unless we're absolutely sure we want this to be an error.

physutils/tests/test_physio.py

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

I modified the history such that it will display the full function in the module

This way we will be able to integrate phys2denoise functions in the history (or physioqc or more). However the supported modules shall all be included inside load_history()

…f different modules

m-miedema · 2024-06-20T15:14:08Z

physutils/io.py

+    # TODO: These will need to be imported in order to replay history from this module. Unless another way is found
+    # import peakdet
+    # import phys2denoise


We'll need to be cautious in how/whether we implement this to avoid introducing unnecessary dependencies! I think it will be good to open another issue to address this.

In ff22aec a warning is added for not installed modules. Would it be useful to implement for example:

Having a history.json that uses both peakdet and physutils

Only having installed peakdet

Perform only the peakdet operations

I'd guess that the other way around wouldn't be useful. Still, even this case wouldn't seem too useful either, but it could make sense to implement

Wdyt @m-miedema ?

Sorry, I missed this comment until now @maestroque ! The way I'm understanding what you're proposing, I agree that it wouldn't be too useful to implement.

m-miedema

This looks good to me, see some of my individual comments for things to consider as we move forward. I am not very sure about carrying the neurokit integration forward, however. @me-pic @smoia @sangfrois do you have any thoughts on long-term support for this functionality?

smoia · 2024-06-20T15:26:19Z

I think we should bring it forward, but with local imports in the neeed functions so that NK stays an optional dependency

maestroque · 2024-06-24T16:09:40Z

I believe this should be ready as well now. The only thing that remains is to add the automations for publishing. I'll add the yaml files and then @smoia adds the PyPI credentials etc. as GH secrets whenever he is available

@m-miedema @me-pic

…lish) and issue templates

me-pic · 2024-06-25T14:02:23Z

physutils/tests/test_io.py

+
+    # make physio object and perform some operations
+    phys = io.load_physio(fname, fs=1000.0)
+    filt = filter_physio(phys, [5.0, 15.0], "bandpass")


I got an error with the filter_physio function when running pytest:
*** AttributeError: 'numpy.float64' object has no attribute '_history'

Can anybody reproduce this error ?

me-pic · 2024-06-25T14:06:35Z

physutils/physio.py

+    copy_history=True,
+    copy_metadata=True,
+    copy_suppdata=True,
+    copy_label=True,


copy_label, copy_physio_type and copy_computed_metrics don't seem to be used in that function

fixed that as well

me-pic

LGTM !

maestroque · 2024-07-10T14:28:59Z

@me-pic @m-miedema I'm also working on the BIDS support for creating Physio objects (not pushed yet), should it be in this PR or merge this and open another?

me-pic · 2024-07-10T14:37:17Z

@maestroque I think we could merge this one and open another PR to integrate BIDS support !

maestroque · 2024-07-11T11:03:03Z

Ok then, merging

A few mods stemming from PR convo

maestroque added 4 commits June 5, 2024 16:38

Add pre-commit hooks yaml and update setup.cfg

01627f5

Transfer Physio class and io.py functions

7f15785

Add plot_physio as a Physio method

f89f5a3

Add make_operation, new_physio_like and check_physio functions

1723e65

Delete placeholder phys2bids Physio class files

bc8f054

maestroque added 2 commits June 17, 2024 13:10

Minor fix

c909e91

Add unit tests and minor fixes

6c68171

me-pic reviewed Jun 18, 2024

View reviewed changes

maestroque added 2 commits June 18, 2024 15:58

Add physio_type and label as Physio attributes

e296087

Add io functions tests, history needs rework to include module name

1c10237

maestroque added 2 commits June 19, 2024 23:21

Add full module name and function call to history, allowing for use o…

c2a8776

…f different modules

Add logging assertions in unit tests

4c3d209

m-miedema reviewed Jun 20, 2024

View reviewed changes

m-miedema approved these changes Jun 20, 2024

View reviewed changes

m-miedema added the Majormod label Jun 20, 2024

smoia added Minormod MInormod-breaking and removed Majormod Minormod labels Jun 20, 2024

maestroque changed the title ~~[WIP] Migrate Physio class from peakdet~~ Migrate Physio class from peakdet Jun 23, 2024

maestroque added 4 commits June 23, 2024 19:26

Style fix

ff22aec

Additional error hanndling and docs for Physio

018fe21

Add Physio computed_metrics property

64cc90f

Minor fixes for added Physio properties

68df7a3

maestroque added 3 commits June 24, 2024 21:51

Minor log message change

045fe0f

Add possibility for multiple function outputs with make_operation

8085a5f

Add .autorc, automation workflows (author assign, label, release, pub…

0ae56e1

…lish) and issue templates

me-pic reviewed Jun 25, 2024

View reviewed changes

maestroque added 3 commits July 9, 2024 13:39

Add pre-commit as a devtools dependency

9ac0fe0

Physio history appending fix

5a42d22

Fix new_physio_like function

e7c66b4

me-pic approved these changes Jul 10, 2024

View reviewed changes

maestroque self-assigned this Jul 10, 2024

maestroque merged commit 2ef56b8 into physiopy:master Jul 11, 2024

smoia added the released label Jul 11, 2024

smoia mentioned this pull request Oct 9, 2024

Add BIDS reading support and prepare input loading for pydra workflow #7

Merged

18 tasks

me-pic pushed a commit to me-pic/physutils that referenced this pull request Oct 25, 2024

Merge pull request physiopy#2 from smoia/add-pydra

1a72423

A few mods stemming from PR convo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Physio class from peakdet #2

Migrate Physio class from peakdet #2

maestroque commented Jun 5, 2024

me-pic commented Jun 13, 2024

maestroque commented Jun 13, 2024

maestroque commented Jun 17, 2024

m-miedema commented Jun 17, 2024

me-pic left a comment •

edited

Loading

me-pic Jun 17, 2024

maestroque Jun 18, 2024

m-miedema Jun 20, 2024 •

edited

Loading

m-miedema Jun 20, 2024

maestroque Jun 24, 2024

me-pic Jun 18, 2024

maestroque Jun 18, 2024

m-miedema Jun 20, 2024

maestroque commented Jun 19, 2024

m-miedema Jun 20, 2024

maestroque Jun 23, 2024

m-miedema Jul 11, 2024

m-miedema left a comment

smoia commented Jun 20, 2024

maestroque commented Jun 24, 2024

me-pic Jun 25, 2024

maestroque Jul 9, 2024

me-pic Jun 25, 2024

maestroque Jul 9, 2024

me-pic left a comment

maestroque commented Jul 10, 2024

me-pic commented Jul 10, 2024 •

edited

Loading

maestroque commented Jul 11, 2024

Migrate Physio class from peakdet #2

Migrate Physio class from peakdet #2

Conversation

maestroque commented Jun 5, 2024

Changes

me-pic commented Jun 13, 2024

maestroque commented Jun 13, 2024

maestroque commented Jun 17, 2024

m-miedema commented Jun 17, 2024

me-pic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-miedema Jun 20, 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

maestroque commented Jun 19, 2024

Choose a reason for hiding this comment

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

smoia commented Jun 20, 2024

maestroque commented Jun 24, 2024

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

me-pic left a comment

Choose a reason for hiding this comment

maestroque commented Jul 10, 2024

me-pic commented Jul 10, 2024 • edited Loading

maestroque commented Jul 11, 2024

me-pic left a comment •

edited

Loading

m-miedema Jun 20, 2024 •

edited

Loading

me-pic commented Jul 10, 2024 •

edited

Loading