Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add BIDS reading support and prepare input loading for pydra workflow #7
Changes from 6 commits
557513e
52cc61e
9c224f0
05d90ea
cb67b7c
e10c9b2
717ff0e
db56954
6c9a6d2
67ee789
3a08510
1cd7539
a19bc19
34fc89d
94b711c
1a72423
f2a2d92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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. Thattask
would be a decorator that returns the function.(like here: https://github.com/BMRRgroup/wfTFI/blob/ec5a62cf2ffb347c752293e6234b7c89cebe711d/QSM.py#L7C1-L13C1)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smoia feel free to check, I implemented it with just a wrapper and it seems to work
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.
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.
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
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.
Kudos on changing name to
generate_physio
or something similar.Double kudos on adding a good docstring for explanations!
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.
I'm a bit confused about these two lines, was there something you were trying to check?
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.
pytest is failing..
inputs
is not an attribute included in the Physio class, norinput_file
ormode
...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.
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 successfulThere 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.
This is failing since
bids_channel
is not a valid argument in thegenerate_physio
functionThere 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.
Ok just realized looking at the previous comments that
bids_channel
was changed tocol_physio_type
in generate_physio, but thetest_tasks.py
have not been update accordinglyThere 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.
Move this to an extra
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.
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.