Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"code",
"ideas",
"infra",
"review",
"review"
]
},
{
Expand All @@ -38,7 +38,7 @@
"data",
"ideas",
"infra",
"projectManagement",
"projectManagement"
]
},
{
Expand All @@ -51,6 +51,20 @@
"review",
"test"
]
},
{
"login": "maestroque",
"name": "George Kikas",
"avatar_url": "https://avatars.githubusercontent.com/u/74024609?v=4",
"profile": "https://github.com/maestroque",
"contributions": [
"code",
"ideas",
"infra",
"bug",
"test",
"review"
Copy link
Member

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.

]
}
],
"contributorsPerLine": 7,
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
<td align="center"><a href="http://rossmarkello.com"><img src="https://avatars0.githubusercontent.com/u/14265705?v=4" width="100px;" alt=""/><br /><sub><b>Ross Markello</b></sub></a><br /><a href="https://github.com/physiopy/phys2bids/commits?author=rmarkello" title="Code">💻</a> <a href="#ideas-rmarkello" title="Ideas, Planning, & Feedback">🤔</a> <a href="#infra-rmarkello" title="Infrastructure (Hosting, Build-Tools, etc)">🚇</a> <a href="https://github.com/physiopy/phys2bids/pulls?q=is%3Apr+reviewed-by%3Armarkello" title="Reviewed Pull Requests">👀</a></td>
<td align="center"><a href="https://github.com/smoia"><img src="https://avatars3.githubusercontent.com/u/35300580?v=4" width="100px;" alt=""/><br /><sub><b>Stefano Moia</b></sub></a><br /><a href="https://github.com/physiopy/phys2bids/commits?author=smoia" title="Code">💻</a> <a href="#data-smoia" title="Data">🔣</a> <a href="#ideas-smoia" title="Ideas, Planning, & Feedback">🤔</a> <a href="#infra-smoia" title="Infrastructure (Hosting, Build-Tools, etc)">🚇</a> <a href="#projectManagement-smoia" title="Project Management">📆</a></td>
<td align="center"><a href="https://github.com/eurunuela"><img src="https://avatars0.githubusercontent.com/u/13706448?v=4" width="100px;" alt=""/><br /><sub><b>Eneko Uruñuela</b></sub></a><br /><a href="https://github.com/physiopy/phys2bids/commits?author=eurunuela" title="Code">💻</a> <a href="https://github.com/physiopy/phys2bids/pulls?q=is%3Apr+reviewed-by%3Aeurunuela" title="Reviewed Pull Requests">👀</a> <a href="https://github.com/physiopy/phys2bids/commits?author=eurunuela" title="Tests">⚠️</a></td>
<td align="center"><a href="https://github.com/maestroque"><img src="https://avatars.githubusercontent.com/u/74024609?v=4?s=100" width="100px;" alt="George Kikas"/><br /><sub><b>George Kikas</b></sub></a><br /><a href="https://github.com/physiopy/phys2denoise/commits?author=maestroque" title="Code">💻</a> <a href="#ideas-maestroque" title="Ideas, Planning, & Feedback">🤔</a> <a href="#infra-maestroque" title="Infrastructure (Hosting, Build-Tools, etc)">🚇</a> <a href="https://github.com/physiopy/phys2denoise/issues?q=author%3Amaestroque" title="Bug reports">🐛</a> <a href="https://github.com/physiopy/phys2denoise/commits?author=maestroque" title="Tests">⚠️</a> <a href="https://github.com/physiopy/phys2denoise/pulls?q=is%3Apr+reviewed-by%3Amaestroque" title="Reviewed Pull Requests">👀</a></td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about this, I'll regenerate the table later

</tr>
</table>

Expand Down
85 changes: 85 additions & 0 deletions physutils/tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import logging
from functools import wraps

from bids import BIDSLayout
from loguru import logger

from physutils.io import load_from_bids, load_physio
from physutils.physio import Physio

LGR = logging.getLogger(__name__)
LGR.setLevel(logging.DEBUG)

try:
import pydra

pydra_imported = True
except ImportError:
pydra_imported = False


def mark_task(pydra_imported=pydra_imported):
def decorator(func):
if pydra_imported:
# If the decorator exists, apply it
@wraps(func)
def wrapped_func(*args, **kwargs):
logger.debug(f"Creating pydra task for {func.__name__}")
return pydra.mark.task(func)(*args, **kwargs)

return wrapped_func
# Otherwise, return the original function
return func

return decorator
Copy link
Member

@smoia smoia Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may work, but I honestly was thinking about something like:

def task(func):
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        return func(*args, **kwargs)
    return wrapper

In a utils module, and then here:.

try:
    from pydra.mark import task
except ImportError:
    from .utils import task



def is_bids_directory(directory):
try:
# Attempt to create a BIDSLayout object
_ = BIDSLayout(directory)
return True
except Exception as e:
# Catch other exceptions that might indicate the directory isn't BIDS compliant
logger.error(
f"An error occurred while trying to load {directory} as a BIDS Layout object: {e}"
)
return False


@mark_task(pydra_imported=pydra_imported)
def transform_to_physio(
input_file: str, mode="physio", fs=None, bids_parameters=dict(), bids_channel=None
Copy link
Member

@m-miedema m-miedema Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, it's ok as transform_to_physio, as even in case it reads a .phys file, it transforms a pickled Physio object file, into a Physio object instance to be used in further tasks.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos on changing name to generate_physio or something similar.
Double kudos on adding a good docstring for explanations!

) -> Physio:
if not pydra_imported:
LGR.warning(
Copy link
Member

@smoia smoia Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LGR.warning(
LGR.debug(

"Pydra is not installed, thus transform_to_physio is not available as a pydra task. Using the function directly"
)
LGR.debug(f"Loading physio object from {input_file}")
if not fs:
fs = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about these two lines, was there something you were trying to check?


if mode == "auto":
if input_file.endswith((".phys", ".physio", ".1D", ".txt", ".tsv", ".csv")):
mode = "physio"
elif is_bids_directory(input_file):
mode = "bids"
else:
raise ValueError(
"Could not determine mode automatically, please specify mode"
)
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,12 @@
{
"SamplingFrequency": 10000.0,
"StartTime": -3,
"Columns": [
"time",
"respiratory_chest",
"trigger",
"cardiac",
"respiratory_CO2",
"respiratory_O2"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"SamplingFrequency": 10000.0,
"StartTime": -3,
"Columns": [
"time",
"respiratory_chest",
"trigger",
"cardiac",
"respiratory_CO2",
"respiratory_O2"
]
}
107 changes: 107 additions & 0 deletions physutils/tests/test_tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
"""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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest is failing.. inputs is not an attribute included in the Physio class, nor input_file or mode...

Copy link
Contributor Author

@maestroque maestroque Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputs is a pydra task attribute specifying the input struct of the task, it may be that the new changes making pydra optional broke the tests. Before the tests were successful


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",
Copy link
Contributor

@me-pic me-pic Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing since bids_channel is not a valid argument in the generate_physio function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok just realized looking at the previous comments that bids_channel was changed to col_physio_type in generate_physio, but the test_tasks.py have not been update accordingly

)

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)


def test_transform_to_physio_auto():
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="auto",
bids_parameters=bids_parameters,
bids_channel="cardiac",
me-pic marked this conversation as resolved.
Show resolved Hide resolved
)

assert task.inputs.input_file == bids_dir
assert task.inputs.mode == "auto"
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)


def test_transform_to_physio_auto_error(caplog):
bids_dir = os.path.abspath("physutils/tests/data/non-bids-dir")
task = tasks.transform_to_physio(
input_file=bids_dir,
mode="auto",
bids_channel="cardiac",
)

assert task.inputs.input_file == bids_dir
assert task.inputs.mode == "auto"
assert task.inputs.fs is None
assert task.inputs.bids_channel == "cardiac"

try:
task()
except Exception:
assert caplog.text.count("ERROR") == 1
assert (
caplog.text.count(
"dataset_description.json' is missing from project root. Every valid BIDS dataset must have this file."
)
== 1
)
4 changes: 2 additions & 2 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ classifiers =
License :: OSI Approved :: Apache Software License
Programming Language :: Python :: 3
license = Apache-2.0
description = Set of utilities meant to be used with Physiopy's libraries
description = Set of utilities meant to be used with Physiopy libraries
long_description = file:README.md
long_description_content_type = text/markdown; charset=UTF-8
platforms = OS Independent
Expand All @@ -23,8 +23,8 @@ python_requires = >=3.6.1
install_requires =
matplotlib
numpy >=1.9.3
scipy
loguru
pydra
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to an extra

pybids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, we need to move this to extras as well (it comes with SO MANY dependencies). Could you please open an issue to generically move as many dependencies as possible to extra dependencies? Not now, but it would be good to do so later.

tests_require =
pytest >=3.6
Expand Down