Add main workflow of phys2denoise #17

smoia · 2020-11-12T23:52:52Z

Closes #14

Proposed Changes

Main structure of phys2denoise
parser

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!

Co-authored-by: Stefano Moia <[email protected]>

phys2denoise.py

@62442katieb

Co-authored-by: @62442katieb <[email protected]>

smoia · 2020-11-25T22:19:58Z

We're ready for review!
Apologies for the time it took me to get here.
Question: who contributed to the logger part in phys2bids? @eurunuela and @vinferrer ?

In any case, please have a look! @tsalo @eurunuela @62442katieb, let's see if we can merge this soon(ish)!

phys2denoise.py

smoia · 2020-11-25T22:29:50Z

Also let me be clear: this is a little, small, first step toward having something that given an input returns an output that makes sense (together with the other bunch of PRs). Nowhere near the state of development we want to get to though!

For this reason I'm using the minormod-breaking label. We're still in alpha after all.

phys2denoise.py

eurunuela

LGTM! Thanks @smoia !

eurunuela · 2020-11-26T09:33:41Z

Oh, I almost forgot. You should update the label if you think this is a majormod (I think it is).

smoia · 2020-11-27T14:04:30Z

Oh, I almost forgot. You should update the label if you think this is a majormod (I think it is).

Yup, you're right in that this is a majormod, however I thought that the general approach we wanted to follow (phys2bids#192) was not to go in the 1.0 + versioning until we leave beta stage. That's why I'm adopting the minormod-breaking label (after the suggestions in phys2bids#192)

eurunuela · 2020-11-27T14:08:53Z

Yup, you're right in that this is a majormod, however I thought that the general approach we wanted to follow (phys2bids#192) was not to go in the 1.0 + versioning until we leave beta stage. That's why I'm adopting the minormod-breaking label (after the suggestions in phys2bids#192)

Sounds good! It's also good that we have it written on this thread for future reference.

vinferrer · 2021-02-11T21:46:21Z

phys2denoise.py

+                                                                  **args)
+        elif metrics == 'retroicor_resp':
+            args = select_input_args(compute_retroicor_regressors, metric_args)
+            args['resp'] = True


Doesn't make more sense to modify this so we don't have two if conditions, one here and another one in the retroicor code?

What do you mean @vinferrer ? The two ifs do different things (Or you mean to recode the three functions in retroicor intotwo? But then there's quite a lot of lines that are common...)

Okay, after seeing that you don't input the metrics argument directly, why don't you put this line inside select_input_args I think you can reduce this if statement at least, another thing is that instead of creating the dictionary entry args['card'] = True when metrics == 'retroicor_card' you could use directly the metrics == 'retroicor_card' inside compute_retroicor_regressors. Do you follow? or should I do suggestions in both PR's?

I think we can avoid an extra if statement

I mean, you have a function called select_input_args I would include the type of metric detection inside of this function rather than in the main workflow

phys2denoise.py

vinferrer · 2021-02-15T08:32:38Z

phys2denoise.py

+        else:
+            args = select_input_args(metric, metric_args)
+            print_metric_call(metric, args)
+            regr[f'{metric}'] = metric(physio, **args)


Also, what function is this?

…rkflow

This reverts commit 5adc4f5.

tsalo

Just a couple of questions/comments. I imagine we'll need to resolve some conflicts between the parameter names in this PR and those in #19, but that's not a big deal and shouldn't slow us down.

phys2denoise/phys2denoise.py

phys2denoise/cli/run.py

smoia · 2021-02-25T17:55:29Z

🚀 PR was released in 0.1.0 🚀

smoia and others added 6 commits November 11, 2020 15:42

Add first skeleton draft

ab66ba8

add parser

5f685eb

Update phys2denoise/cli/run.py

2fa576f

Co-authored-by: Stefano Moia <[email protected]>

Update

6142d30

Merge branch 'master' of https://github.com/ineschh/phys2denoise

ae489ad

More skeleton

8e521f3

eurunuela reviewed Nov 13, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

ineschh added 2 commits November 13, 2020 10:10

More updates

4c265e3

Suggested changes

8bd4b8f

tsalo reviewed Nov 13, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

Import metrics

7b950b6

tsalo reviewed Nov 13, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

smoia added Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility Enhancement New feature or request and removed Enhancement New feature or request labels Nov 13, 2020

smoia added 5 commits November 14, 2020 11:38

First skeleton version

6090b6f

Remove unused libraries and improve function argument

195c369

Input args to metrics using inspect.signature()

298acaf

Change log path

bd340be

Change inspection of metrics and add a logger function

cb8b42e

smoia requested a review from 62442katieb November 25, 2020 22:14

Recognising planning contribution to @62442katieb

9c1bd60

Co-authored-by: @62442katieb <[email protected]>

smoia marked this pull request as ready for review November 25, 2020 22:18

tsalo reviewed Nov 25, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

tsalo reviewed Nov 25, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

tsalo reviewed Nov 25, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

tsalo reviewed Nov 25, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

tsalo reviewed Nov 25, 2020

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

eurunuela approved these changes Nov 26, 2020

View reviewed changes

metric_args into dict

1541149

vinferrer reviewed Feb 11, 2021

View reviewed changes

vinferrer reviewed Feb 15, 2021

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

vinferrer reviewed Feb 15, 2021

View reviewed changes

phys2denoise.py Outdated Show resolved Hide resolved

vinferrer reviewed Feb 15, 2021

View reviewed changes

Stefano Moia added 6 commits February 25, 2021 15:46

Remove extra "version" imports

636f585

Change metric_args into **kwargs

36abfb8

Move metric call log to metrics.utils instead of in the main workflow.

46c1e0c

Merge branch 'ineschh/master' into enh/skeleton

7c25d9c

Move the bash call saving to a function on its own out of the main wo…

44fde8c

…rkflow

Move main workflow in the right place

570619d

tsalo mentioned this pull request Feb 25, 2021

Allow YAML-based config file with input arguments from/for workflow #29

Open

Stefano Moia added 3 commits February 25, 2021 17:07

Add parser

3a66afa

Change call to retroicor into argument selection of type

5adc4f5

Revert "Change call to retroicor into argument selection of type"

6e7f7ee

This reverts commit 5adc4f5.

tsalo reviewed Feb 25, 2021

View reviewed changes

phys2denoise/phys2denoise.py Outdated Show resolved Hide resolved

phys2denoise/cli/run.py Outdated Show resolved Hide resolved

Better retroicor call

76fe437

tsalo reviewed Feb 25, 2021

View reviewed changes

phys2denoise/cli/run.py Outdated Show resolved Hide resolved

Stefano Moia added 2 commits February 25, 2021 18:49

Deal better with retroicor outputs

d767359

Remove unused code

1ca8780

smoia assigned tsalo and smoia Feb 25, 2021

smoia merged commit ad51a00 into physiopy:master Feb 25, 2021

smoia added the released This issue/pull request has been released label Feb 25, 2021

smoia deleted the enh/skeleton branch February 25, 2021 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add main workflow of phys2denoise #17

Add main workflow of phys2denoise #17

smoia commented Nov 12, 2020 •

edited

Loading

smoia commented Nov 25, 2020

smoia commented Nov 25, 2020

eurunuela left a comment

eurunuela commented Nov 26, 2020

smoia commented Nov 27, 2020

eurunuela commented Nov 27, 2020

vinferrer Feb 11, 2021

smoia Feb 11, 2021

vinferrer Feb 15, 2021

vinferrer Feb 15, 2021

vinferrer Feb 15, 2021

vinferrer Feb 15, 2021

smoia Feb 25, 2021

tsalo left a comment

smoia commented Feb 25, 2021

Add main workflow of phys2denoise #17

Add main workflow of phys2denoise #17

Conversation

smoia commented Nov 12, 2020 • edited Loading

Proposed Changes

Change Type

Checklist before review

smoia commented Nov 25, 2020

smoia commented Nov 25, 2020

eurunuela left a comment

Choose a reason for hiding this comment

eurunuela commented Nov 26, 2020

smoia commented Nov 27, 2020

eurunuela commented Nov 27, 2020

vinferrer Feb 11, 2021

Choose a reason for hiding this comment

smoia Feb 11, 2021

Choose a reason for hiding this comment

vinferrer Feb 15, 2021

Choose a reason for hiding this comment

vinferrer Feb 15, 2021

Choose a reason for hiding this comment

vinferrer Feb 15, 2021

Choose a reason for hiding this comment

vinferrer Feb 15, 2021

Choose a reason for hiding this comment

smoia Feb 25, 2021

Choose a reason for hiding this comment

tsalo left a comment

Choose a reason for hiding this comment

smoia commented Feb 25, 2021

smoia commented Nov 12, 2020 •

edited

Loading