Add Physio object generation from BIDS #4

maestroque · 2024-07-11T11:27:34Z

Closes #3

Proposed Changes

Add a function to load BIDS structures and automatically generate Physio objects
Add an MRI data object to store trigger parameters, universal to all generated Physio objects

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-07-11T12:45:47Z

See my comment on the corresponding issue, I think we'll need to add more flexibility to column naming for this to be useful.

maestroque · 2024-07-16T15:49:16Z

@m-miedema it's fixed for the time being. I have implemented string literals searching, to give the physio_type property that is used in some functions.

m-miedema · 2024-07-18T12:12:35Z

This looks reasonable to me! Let me know when it's ready for review :)

me-pic · 2024-07-22T14:45:38Z

Hi @maestroque ! For the addition of unit tests for those new functions, I've talked with a colleague and she recommended to just generate the structure on the fly when we run pytest (and generate random number for the timeseries if we even need to), if we are just concerned about the data structure ! If we are more interested about the content of those files and need actual real data you can take a look at that BIDS app

maestroque · 2024-07-22T15:48:53Z

Thanks @me-pic ! Sounds reasonable to me

me-pic

@maestroque Nice work on that PR !

me-pic · 2024-07-22T20:07:10Z

physutils/io.py

+
+    for col in columns:
+        col_physio_type = None
+        if any([x in col for x in ["cardiac", "ppg", "ecg", "card"]]):


@maestroque Would it be possible to change for col.lower() ?

Nice catch!

me-pic · 2024-07-22T20:28:11Z

physutils/io.py

 from loguru import logger

 from physutils import physio

 EXPECTED = ["data", "fs", "history", "metadata"]


+def load_from_bids(


Would it be possible to add the parameter recording=None ? The reason is that according to the current version of the BIDS spec physio data with different sampling rate need to be saved under different file names using the recording entity to differentiate them apart.

And recording should be added when you call layout.get

Got it, done!

me-pic · 2024-07-22T20:32:07Z

physutils/io.py

+    if not op.exists(bids_path):
+        raise FileNotFoundError(f"Provided path {bids_path} does not exist")
+
+    layout = BIDSLayout(bids_path)


Not sure if there is a reason not to do that but I think we should specify layout = BIDSLayout(bids_path, validate=False). I tested the current function with a BIDS dataset containing multiple physio files saved under names using different label values with the recording entity (ex. recording-pulse and recording-breathing ), and if validate=False is not specified, it just gives me an empty list after layout.get()

This change + the addition of the recording parameter would allow people with BIDS dataset containing physio data with different sampling rate to still be able to use those functionalities

maestroque · 2024-08-09T19:55:19Z

@me-pic the unit test is ready. I'll address your other comments shortly and it should be good

maestroque · 2024-08-09T20:02:40Z

setup.cfg

I guess we should also change the matplotlib >=3.9 line to non-constraining?

Yes, good catch !

me-pic

Thank you @maestroque for integrating previous comments ! I've added some more 😅 Should be pretty straight forward to integrate, but let me know if you need help !

me-pic · 2024-08-12T13:57:32Z

physutils/io.py

+
+    for col in columns:
+        col_physio_type = None
+        if any([x in col.lower() for x in ["cardiac", "ppg", "ecg", "card"]]):


@m-miedema Do you think it would be reasonable to add "pulse" as a valid column name for "cardiac" ? And also "o2" and "co2" for "respiratory" ?

Although O2 and CO2 reflect respiration, I don't think they should be labelled as respiratory! This is mainly because the respiratory response function should not be used for these types of data (see e.g. the fMRI literature on end-tidal response functions), and users may become confused with putting this data in the same category as a belt measurement.

For pulse, I've never personally seen it used as a naming convention, but why not!

me-pic · 2024-08-12T14:05:29Z

physutils/tests/utils.py

+    run_id = "01"
+
+    bids_dir = pjoin(
+        data_dir, "bids-dir", f"sub-{subject_id}", f"ses-{session_id}", "func"


I think we should make sure that the test covers the possibility to have the recording entity mentioned in the filename. Ex. sub-006_ses-03_task-test_run-01_recording-cardiac_physio.tsv.gz would be a valid filename according to the BIDS standard. And since we also included a recording parameter in the load_from_bids function

Sorry @maestroque I think my comment was not clear. The tests should cover the possibility to have filenames with the recording entity as well as filenames that does not include that entity.

me-pic · 2024-08-12T14:05:54Z

setup.cfg

Yes, good catch !

me-pic · 2024-08-12T14:08:34Z

physutils/physio.py

    label = ref_physio.label if copy_label else None
    physio_type = ref_physio.physio_type if copy_physio_type else None
-    computed_metrics = list(ref_physio.computed_metrics) if copy_computed_metrics else []
+    computed_metrics = (
+        dict(ref_physio.computed_metrics) if copy_computed_metrics else []


Should it be dict(ref_physio.computed_metrics) if copy_computed_metrics else {} instead ?

me-pic · 2024-08-12T14:12:44Z

physutils/physio.py

@@ -542,3 +544,55 @@ def neurokit2phys(
            metadata = dict(peaks=peaks)

        return cls(data, fs=fs, metadata=metadata, **kwargs)
+
+
+class MRIConfig:


Maybe I missed it, but is there any test to cover that class ?

No, it is not used yet, because it would need detection of the trigger channel in order to initialize it. I can open an issue defining the next steps for it, and I'll implement it if there is time after the workflow

me-pic · 2024-08-12T14:17:22Z

physutils/tests/test_io.py

+        # The data saved are the ones after t_0 = 0s
+        assert phys_array[col].data.size == 70000
+        assert phys_array[col].fs == 10000.0
+        assert phys_array[col].history[0][0] == "physutils.io.load_from_bids"


Would it be better to delete the bids dataset at the end of the function or just to add the bids-dir directory in the .gitignore ?

m-miedema · 2024-08-14T21:25:34Z

physutils/io.py

+
+    config_file = bids_file[0].get_metadata()
+    fs = config_file["SamplingFrequency"]
+    t_start = config_file["StartTime"]  # noqa


Is there a reason to define the start time here? It seems like it could be integrated into the time column checks, although its not necessary to do that in this PR. Perhaps open an issue to do a QA check and/or integrate the info into the specification of idx_0 in the event of a missing time column.

I integrated it into the idx_0 definition

physutils/physio.py

m-miedema

Hi @maestroque ! This looks very nice and well-organized so far, I left a few comments to add to what @me-pic has included in her review. The main thing I want to understand better is the relationship between the MRIcfg and Physio objects, and whether it's relevant to extract any information from the trigger column itself (rather than the sidecar file). I don't think there's much in the MRI class that can be generated from the trigger column, other than the TR and number of scans, so while you can confirm this is correct from the sidecar, I don't think it's necessary to do so. I'm also wondering if you intend to e.g point the Physio object to its corresponding MRIcfg object in some way while you're setting them up - would this be helpful in metric calls later on?

maestroque · 2024-08-19T15:43:27Z

@me-pic I integrated your comments, so it should be good from that side!

@m-miedema I also addressed your comments on StartTime and n_slices. Regarding the use of the MRIConfig class and Physio, what you say is true, that was the intended use of the new class as we discussed in the past. It is pending the extraction of data from the trigger channel and the integration into the Physio object. I will open an issue about this, but I will tackle it if time allows after the workflow

maestroque · 2024-08-19T15:59:28Z

Other than that, it should be gtg

me-pic

Thank you @maestroque for integrating the changes ! I've added to more comments, but once those are addressed I think we could merge 🚀

me-pic · 2024-08-19T16:38:20Z

physutils/tests/utils.py

+    run_id = "01"
+
+    bids_dir = pjoin(
+        data_dir, "bids-dir", f"sub-{subject_id}", f"ses-{session_id}", "func"


Sorry @maestroque I think my comment was not clear. The tests should cover the possibility to have filenames with the recording entity as well as filenames that does not include that entity.

me-pic · 2024-08-19T16:42:41Z

physutils/tests/utils.py

+    df.to_csv(tsv_file, sep="\t", index=False, header=False, float_format="%.8e")
+
+    # Compress tsv file into tsv.gz
+    tsv_gz_file = pjoin(


I'm not sure I understand why the file is first saved as a tsp, then loaded and saved again as a compressed file. I think you can just directly save df as a tsv.gz since that is the required format according to the BIDS spec.

Yes, you are right, it was left as such from debugging

maestroque · 2024-08-21T14:27:02Z

@me-pic I think we are now ready, let me know if you have anything else in mind

me-pic

LGTM 🎉

m-miedema · 2024-08-21T21:12:13Z

Great work, thanks @maestroque !

smoia · 2024-08-25T13:33:19Z

🚀 PR was released in 0.2.1 🚀

Add preliminary load_from_bids function

e6b9673

maestroque added the MInormod-breaking label Jul 11, 2024

maestroque requested review from smoia, m-miedema and me-pic July 11, 2024 11:27

maestroque self-assigned this Jul 11, 2024

github-actions bot added the Internal label Jul 11, 2024

maestroque removed the Internal label Jul 11, 2024

Add MRIConfig class

ecb73be

github-actions bot added the Internal label Jul 16, 2024

maestroque added 3 commits July 16, 2024 18:18

Fixes for working load_from_bids function

3440fe8

Remove supported columns and add detection for known columns

344ce3d

load_from_bids: Dictionary keys named after column names

f6b3d9f

load_from_bids: return trigger as Physio

efeaccc

me-pic requested changes Jul 22, 2024

View reviewed changes

maestroque changed the title ~~[WIP] Add Physio object generation from BIDS~~ Add Physio object generation from BIDS Jul 23, 2024

Add BIDS structure creation test utils function

6bafa70

github-actions bot added the Testing label Aug 9, 2024

Add load_from_bids unit test

c281917

Review fixes

1b0deaa

maestroque commented Aug 9, 2024

View reviewed changes

Fix computed_metrics attribute initialization

0c324c5

maestroque mentioned this pull request Aug 10, 2024

Integrate physutils - Physio object usage physiopy/phys2denoise#54

Merged

18 tasks

Fix new_physio_like computed_metrics copying bug

2b0deb9

me-pic reviewed Aug 12, 2024

View reviewed changes

m-miedema reviewed Aug 14, 2024

View reviewed changes

physutils/physio.py Outdated Show resolved Hide resolved

m-miedema requested changes Aug 14, 2024

View reviewed changes

maestroque added 3 commits August 19, 2024 17:55

Minor improvements

038be86

matplotlib dependency constraint removal

82d6ee9

Integrate StartTime BIDS physio parameter

bbe49a6

Minor fix

d82ef72

me-pic requested changes Aug 19, 2024

View reviewed changes

[test] BIDS structure creation utility function fix

ffa8e71

me-pic approved these changes Aug 21, 2024

View reviewed changes

m-miedema mentioned this pull request Aug 21, 2024

Integrate the MRIConfig and Physio classes #5

Open

m-miedema approved these changes Aug 21, 2024

View reviewed changes

m-miedema merged commit e0562d4 into physiopy:master Aug 21, 2024
1 check passed

smoia added the released label Aug 25, 2024

Add Physio object generation from BIDS #4

Add Physio object generation from BIDS #4

Conversation

maestroque commented Jul 11, 2024 • edited Loading

Proposed Changes

Change Type

Checklist before review

m-miedema commented Jul 11, 2024

maestroque commented Jul 16, 2024

m-miedema commented Jul 18, 2024

me-pic commented Jul 22, 2024

maestroque commented Jul 22, 2024

me-pic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-pic Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

me-pic Jul 22, 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

maestroque commented Aug 9, 2024

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

Choose a reason for hiding this comment

m-miedema Aug 14, 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

m-miedema left a comment

Choose a reason for hiding this comment

maestroque commented Aug 19, 2024

maestroque commented Aug 19, 2024

me-pic 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

maestroque commented Aug 21, 2024

me-pic left a comment

Choose a reason for hiding this comment

m-miedema commented Aug 21, 2024

smoia commented Aug 25, 2024

maestroque commented Jul 11, 2024 •

edited

Loading

me-pic Jul 22, 2024 •

edited

Loading

me-pic Jul 22, 2024 •

edited

Loading

m-miedema Aug 14, 2024 •

edited

Loading