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

maestroque · 2024-08-28T13:08:07Z

Closes #9

Proposed Changes

Add a transform_to_physio pydra task to be used in all worfklows

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!

physutils/physio.py

setup.cfg

smoia · 2024-08-28T14:22:55Z

physutils/tasks.py

@@ -0,0 +1,34 @@
+import logging
+
+import pydra


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 from pydra.mark import task, but if it is not found, import task from a stub submodule of physutils. That task would be a decorator that returns the function.
(like here: https://github.com/BMRRgroup/wfTFI/blob/ec5a62cf2ffb347c752293e6234b7c89cebe711d/QSM.py#L7C1-L13C1)

That task would be a decorator that returns the function

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

def mark_task_wrapper(func): try: import pydra logger.debug("Pydra is installed and is currently enabled.") return pydra.mark.task(func) except ImportError: logger.warning("Pydra is not installed and is currently disabled. Please install it to use this module.") return func

@smoia feel free to check, I implemented it with just a wrapper and it seems to work

me-pic · 2024-08-28T15:08:18Z

Other than the changes requested by Stef, LGTM !

m-miedema · 2024-08-29T19:45:14Z

physutils/tasks.py

+
+@pydra.mark.task
+def transform_to_physio(
+    input_file: str, mode="physio", fs=None, bids_parameters=dict(), bids_channel=None


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!

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

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

maestroque · 2024-09-03T14:25:13Z

I also added an auto mode, as per physiopy/phys2denoise#57 (comment)
@smoia @me-pic @m-miedema

smoia · 2024-09-03T16:49:48Z

physutils/tasks.py

+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


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

smoia · 2024-09-03T16:51:19Z

physutils/tasks.py

+    input_file: str, mode="physio", fs=None, bids_parameters=dict(), bids_channel=None
+) -> Physio:
+    if not pydra_imported:
+        LGR.warning(


Suggested change

LGR.warning(

LGR.debug(

smoia · 2024-09-03T16:54:05Z

setup.cfg

    loguru
+    pydra


Move this to an extra

smoia · 2024-09-03T16:56:47Z

setup.cfg

    loguru
+    pydra
    pybids


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.

m-miedema · 2024-09-26T18:22:30Z

@maestroque do you need any help addressing @smoia's comments?

smoia · 2024-10-09T15:52:17Z

@maestroque any progress on this side?

maestroque · 2024-10-09T17:15:30Z

@smoia @m-miedema I didnt have in mind that there was something pending for this sorry. As far as i can tell only the dependencies issue opening

smoia · 2024-10-09T18:43:05Z

physutils/tasks.py

+    if not fs:
+        fs = None


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

…trings, rename transform_to_physio function

smoia · 2024-10-09T19:18:44Z

@maestroque @m-miedema @me-pic I opened a PR to @maestroque 's branch: ~~https://github.com/maestroque/physutils/pull/1/files~~ https://github.com/maestroque/physutils/pull/2/files

It should take care of most issues. Emphasis on should.

smoia · 2024-10-09T19:26:57Z

And I also made a mess out of it - sorry. I shoudl have fixed things now, but the PR is #2 not #1

smoia · 2024-10-09T19:28:46Z

The only thing is that logging is still a bit weird cause it uses logging on one side and loguru on the other. Maybe we should just use logging for the moment?

A few mods stemming from PR convo

smoia

LGTM!

me-pic · 2024-10-16T00:14:26Z

physutils/tests/test_tasks.py

+        input_file=bids_dir,
+        mode="bids",
+        bids_parameters=bids_parameters,
+        bids_channel="cardiac",


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

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

physutils/tests/test_tasks.py

me-pic · 2024-10-16T00:28:08Z

physutils/tests/test_tasks.py

+    assert task.inputs.input_file == physio_file
+    assert task.inputs.mode == "physio"
+    assert task.inputs.fs is None


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

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

physutils/tests/test_tasks.py

me-pic

All the tests in test_tasks.py are failing... Not sure I understand the inputs attribute. Also all the bids_channel need to be changed to col_physio_type

maestroque · 2024-10-16T07:37:21Z

@me-pic @smoia please check the comments, unfortunately I cannot look into this right now. Before the changes it was ready however.

me-pic · 2024-10-25T16:32:38Z

physutils/tasks.py

+# from loguru import logger
+
+try:
+    from pydra import task


@smoia Is it possible that here it should be from pydra.mark import task instead of from pydra import task ?

With that change test_generate_physio_bids_file and test_generate_physio_auto in test_tasks.py are now passing 🤔

Yes that could be it actually. Thank you for catching it!

smoia · 2024-10-25T17:50:48Z

🚀 PR was released in 0.3.0 🚀

maestroque added 6 commits July 22, 2024 16:05

Initial pydra transformation task

557513e

Update pydra task

52cc61e

Pull updates from master

9c224f0

Add BIDS loading option to transform_to_physio task

05d90ea

Working transform_to_physio pydra task and tests

cb67b7c

Verify transform_to_physio for BIDS files

e10c9b2

github-actions bot assigned maestroque Aug 28, 2024

github-actions bot added Testing Internal labels Aug 28, 2024

maestroque added Minormod and removed Testing Internal labels Aug 28, 2024

smoia reviewed Aug 28, 2024

View reviewed changes

physutils/physio.py Outdated Show resolved Hide resolved

maestroque requested review from m-miedema and me-pic August 28, 2024 14:16

smoia reviewed Aug 28, 2024

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

smoia reviewed Aug 28, 2024

View reviewed changes

github-actions bot added Testing Internal labels Aug 28, 2024

Minor review fixes

717ff0e

github-actions bot added the Documentation label Aug 29, 2024

maestroque added 2 commits August 29, 2024 14:11

Add @maestroque as a contributor

db56954

Style fix

6c9a6d2

m-miedema requested changes Aug 29, 2024

View reviewed changes

m-miedema mentioned this pull request Aug 29, 2024

Add pydra tasks, workflow and update CLI physiopy/phys2denoise#57

Open

18 tasks

maestroque added 3 commits September 3, 2024 16:53

Add wrapper for pydra tasks, in order to eliminate pydra requirement

67ee789

Add better logs for pydra package import handling

3a08510

Add transform_to_physio auto mode

1cd7539

smoia reviewed Sep 3, 2024

View reviewed changes

setup.cfg Outdated

loguru

pydra

Copy link

Member

smoia Sep 3, 2024

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

smoia reviewed Sep 3, 2024

View reviewed changes

smoia reviewed Oct 9, 2024

View reviewed changes

smoia added 2 commits October 9, 2024 21:13

Create utils and move there pydra checks and BIDS directory checks

a19bc19

Clean code, remove moved functions, remove pydra mask check, add docs…

34fc89d

…trings, rename transform_to_physio function

Remove extras

94b711c

smoia force-pushed the add-pydra branch from 94b711c to 1cd7539 Compare October 9, 2024 19:24

Merge pull request #2 from smoia/add-pydra

1a72423

A few mods stemming from PR convo

smoia changed the title ~~Add transform_to_physio pydra task~~ Add BIDS reading support and prepare input loading for pydra workflow Oct 12, 2024

smoia approved these changes Oct 12, 2024

View reviewed changes

me-pic reviewed Oct 16, 2024

View reviewed changes

physutils/tests/test_tasks.py Outdated Show resolved Hide resolved

me-pic reviewed Oct 16, 2024

View reviewed changes

physutils/tests/test_tasks.py Show resolved Hide resolved

me-pic requested changes Oct 16, 2024

View reviewed changes

me-pic reviewed Oct 25, 2024

View reviewed changes

Fix test tasks

f2a2d92

me-pic approved these changes Oct 25, 2024

View reviewed changes

me-pic merged commit ab85017 into physiopy:master Oct 25, 2024
1 check passed

smoia added the released label Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maestroque commented Aug 28, 2024 •

edited by smoia

Loading

smoia Aug 28, 2024 •

edited

Loading

maestroque Sep 2, 2024

maestroque Sep 3, 2024

me-pic commented Aug 28, 2024

m-miedema Aug 29, 2024 •

edited

Loading

maestroque Sep 3, 2024

smoia Sep 3, 2024

maestroque commented Sep 3, 2024

smoia Sep 3, 2024 •

edited

Loading

smoia Sep 3, 2024 •

edited

Loading

smoia Sep 3, 2024

smoia Sep 3, 2024

m-miedema commented Sep 26, 2024

smoia commented Oct 9, 2024

maestroque commented Oct 9, 2024

smoia Oct 9, 2024

smoia commented Oct 9, 2024 •

edited

Loading

smoia commented Oct 9, 2024

smoia commented Oct 9, 2024

smoia left a comment

me-pic Oct 16, 2024 •

edited

Loading

me-pic Oct 16, 2024

me-pic Oct 16, 2024

maestroque Oct 16, 2024 •

edited

Loading

me-pic left a comment •

edited

Loading

maestroque commented Oct 16, 2024

me-pic Oct 25, 2024

me-pic Oct 25, 2024

smoia Oct 25, 2024

smoia commented Oct 25, 2024

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

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

Conversation

maestroque commented Aug 28, 2024 • edited by smoia Loading

Proposed Changes

Change Type

Checklist before review

smoia Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-pic commented Aug 28, 2024

m-miedema Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maestroque commented Sep 3, 2024

smoia Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

smoia Sep 3, 2024 • 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 commented Sep 26, 2024

smoia commented Oct 9, 2024

maestroque commented Oct 9, 2024

Choose a reason for hiding this comment

smoia commented Oct 9, 2024 • edited Loading

smoia commented Oct 9, 2024

smoia commented Oct 9, 2024

smoia left a comment

Choose a reason for hiding this comment

me-pic Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maestroque Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

me-pic left a comment • edited Loading

Choose a reason for hiding this comment

maestroque commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoia commented Oct 25, 2024

maestroque commented Aug 28, 2024 •

edited by smoia

Loading

smoia Aug 28, 2024 •

edited

Loading

m-miedema Aug 29, 2024 •

edited

Loading

smoia Sep 3, 2024 •

edited

Loading

smoia Sep 3, 2024 •

edited

Loading

smoia commented Oct 9, 2024 •

edited

Loading

me-pic Oct 16, 2024 •

edited

Loading

maestroque Oct 16, 2024 •

edited

Loading

me-pic left a comment •

edited

Loading