Add BIDS reading support and prepare input loading for pydra workflow #7
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import logging | ||
|
|
||
| import pydra | ||
|
There was a problem hiding this comment. This definitely adds complexity, but since physutils might be used by modules that are not based in pydra, could we try to create a safe import and make it optional, like the one we have for duecredit? https://github.com/physiopy/phys2denoise/blob/master/phys2denoise/due.py The idea would then to try There was a problem hiding this comment.
What do you mean here? I'm not sure I understand how you'd like this to be done (mostly given the duecredit example). I tried doing smth like this, but it didn't really work out There was a problem hiding this comment. @smoia feel free to check, I implemented it with just a wrapper and it seems to work |
||
|
|
||
| from physutils.io import load_from_bids, load_physio | ||
| from physutils.physio import Physio | ||
|
|
||
| LGR = logging.getLogger(__name__) | ||
| LGR.setLevel(logging.DEBUG) | ||
|
|
||
|
|
||
| @pydra.mark.task | ||
| def transform_to_physio( | ||
| input_file: str, mode="physio", fs=None, bids_parameters=dict(), bids_channel=None | ||
|
There was a problem hiding this comment. It makes a lot of sense to add this as a task, great addition to the phys2denoise workflow and to continue to build up physutils :) I'd like to suggest changing the name of bids_channel to col_physio_type to increase consistency with the load_from_bids function - since there is not a lot of documentation within the code itself, I think it's important to try to maintain intuitive links! Beyond that, I'm wondering why the task is named transform_to_physio when it can either transform to physio or read-in an existing physio object. Perhaps e.g. generate_physio would be more appropriate? At minimum I think it's helpful to describe the goal of this task and its potential use cases in the description for the PR. Also, please indicate the change type in the PR description :) Other than these considerations and what Stef mentioned above regarding avoiding a dependency on pydra, this looks good to me! There was a problem hiding this comment. Imo, it's ok as Also maybe it'd be better to change the variable name in load_from_bids? It's not an interface variable (not used outside the function scope) so I don't think we should take this naming as a basis There was a problem hiding this comment. Kudos on changing name to |
||
| ) -> Physio: | ||
| LGR.debug(f"Loading physio object from {input_file}") | ||
| if not fs: | ||
| fs = None | ||
|
There was a problem hiding this comment. I'm a bit confused about these two lines, was there something you were trying to check? |
||
|
|
||
| if mode == "physio": | ||
| if fs is not None: | ||
| physio_obj = load_physio(input_file, fs=fs, allow_pickle=True) | ||
| else: | ||
| physio_obj = load_physio(input_file, allow_pickle=True) | ||
|
|
||
| elif mode == "bids": | ||
| if bids_parameters is {}: | ||
| raise ValueError("BIDS parameters must be provided when loading from BIDS") | ||
| else: | ||
| physio_array = load_from_bids(input_file, **bids_parameters) | ||
| physio_obj = physio_array[bids_channel] | ||
| else: | ||
| raise ValueError(f"Invalid transform_to_physio mode: {mode}") | ||
| return physio_obj | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| """Tests for physutils.tasks and their integration.""" | ||
|
|
||
| import os | ||
|
|
||
| import physutils.tasks as tasks | ||
| from physutils import physio | ||
| from physutils.tests.utils import create_random_bids_structure | ||
|
|
||
|
|
||
| def test_transform_to_physio_phys_file(): | ||
| """Test transform_to_physio task.""" | ||
| physio_file = os.path.abspath("physutils/tests/data/ECG.phys") | ||
| task = tasks.transform_to_physio(input_file=physio_file, mode="physio") | ||
| assert task.inputs.input_file == physio_file | ||
| assert task.inputs.mode == "physio" | ||
| assert task.inputs.fs is None | ||
|
Comment on lines
+14
to
+16
There was a problem hiding this comment. pytest is failing.. There was a problem hiding this comment.
|
||
|
|
||
| task() | ||
me-pic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| physio_obj = task.result().output.out | ||
| assert isinstance(physio_obj, physio.Physio) | ||
| assert physio_obj.fs == 1000 | ||
| assert physio_obj.data.shape == (44611,) | ||
|
|
||
|
|
||
| def test_transform_to_physio_bids_file(): | ||
| """Test transform_to_physio task.""" | ||
| create_random_bids_structure("physutils/tests/data", recording_id="cardiac") | ||
| bids_parameters = { | ||
| "subject": "01", | ||
| "session": "01", | ||
| "task": "rest", | ||
| "run": "01", | ||
| "recording": "cardiac", | ||
| } | ||
| bids_dir = os.path.abspath("physutils/tests/data/bids-dir") | ||
| task = tasks.transform_to_physio( | ||
| input_file=bids_dir, | ||
| mode="bids", | ||
| bids_parameters=bids_parameters, | ||
| bids_channel="cardiac", | ||
|
There was a problem hiding this comment. This is failing since There was a problem hiding this comment. Ok just realized looking at the previous comments that |
||
| ) | ||
|
|
||
| assert task.inputs.input_file == bids_dir | ||
| assert task.inputs.mode == "bids" | ||
| assert task.inputs.fs is None | ||
| assert task.inputs.bids_parameters == bids_parameters | ||
| assert task.inputs.bids_channel == "cardiac" | ||
|
|
||
| task() | ||
|
|
||
| physio_obj = task.result().output.out | ||
| assert isinstance(physio_obj, physio.Physio) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maestroque please put yourself before Vicente (in alphabetical order of family name). You have all of the above, minus review, but plus documentation.
@me-pic and @m-miedema please add yourselves as well, mentoring and review.