-
Notifications
You must be signed in to change notification settings - Fork 19
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 pydra tasks, workflow and update CLI #57
base: master
Are you sure you want to change the base?
Conversation
Note that the PR is currently stemming from |
Also I would like you to enlighten me on retroicor if possible. In the current workflow implementation, the metrics are exported as such: for metric in metrics:
if metric == "retroicor_card":
args = select_input_args(retroicor, kwargs)
args["card"] = True
retroicor_regrs = retroicor(physio, **args)
for vslice in range(len(args["slice_timings"])):
for harm in range(args["n_harm"]):
key = f"rcor-card_s-{vslice}_hrm-{harm}"
regr[f"{key}_cos"] = retroicor_regrs[vslice][:, harm * 2]
regr[f"{key}_sin"] = retroicor_regrs[vslice][:, harm * 2 + 1]
elif metric == "retroicor_resp":
# etc. etc. Shall I keep it this way or is more research needed? I am not familiar with how retroicor is used. |
Keep it this way for now -- the handling of these derivatives is something we'll need to improve but we can circle back after the workflow is in place! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #57 +/- ##
==========================================
- Coverage 53.85% 49.84% -4.02%
==========================================
Files 8 9 +1
Lines 596 644 +48
==========================================
Hits 321 321
- Misses 275 323 +48
|
I have discovered a discrepancy about using loguru within the pydra tasks. E.g. when running this @pydra.mark.task
def compute_metrics(phys, metrics):
if isinstance(metrics, list) or isinstance(metrics, str):
for metric in metrics:
if metric not in _available_metrics:
# print(f"Metric {metric} not available. Skipping")
logger.warning(f"Metric {metric} not available. Skipping")
continue
args = select_input_args(metric, {})
phys = globals()[metric](phys, **args)
logger.info(f"Computed {metric}")
return phys When including the loguru logs, when defining the task as in
While when not including such calls it is not raised. The good thing is that this is only specific to loguru as it seems, because stdlib |
Currently needed to manually install nest_asyncio for testing, but after that the tests pass with the exception of the caplog logging assertion (this is also true for me in physiopy/physutils#7). Still taking a closer look at the rest, will have a full review for you tomorrow! |
Yes I forgot adding the nest_asyncio as a dependency sorry. It is used by the pydra examples, but I also need to verify the use, because currently things are working without it as well What I do to test is running the python3 phys2denoise/cli/run.py -in phys2denoise/tests/data/fake_phys.phys -out exports -cp -rv -hbi -rpv -rp -hr -hrv -md physio -tr 1 -nscans 4 -sl 1.2 3.4 5.6 -win 100 Then you could try different arguments etc. you can use |
@m-miedema what failed tests are you getting? Everything should pass now |
phys2denoise/cli/run.py
Outdated
required.add_argument( | ||
"-md", | ||
"--mode", | ||
dest="mode", | ||
type=str, | ||
help="Format of the input physiological data. Options are: " | ||
"physio or bids. Default is 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.
Let's leave this in for now, but let's also open an issue to remove this option and make a i/o function that automatically recognises the input type based on the given input.
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 shouldn't be to hard, I can incorporate it for this PR
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 where are you regarding that ? If you don't have time for it there is no problem, we can definitely do what Stef suggested ! However, if you decide to do that, can you please add the default value to 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.
Please check this commit @smoia I partially implemented it
phys2denoise/tests/data/ECG.csv
Outdated
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 can help with setting things up, but it would be FAR better if we upload all the data in our OSF repository and remove it from here.
Do you need help with it?
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.
Tbh, I just used the files from peakdet, because in the tests there these files are also uploaded. I mostly use the fake_physio.phys for the tests here which is created on the spot using a util function. I can delete ECG.csv if you want
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.
Transfer the main workflow in workflow.py
as that's the entry point of the CLI. This file should only contain the parser
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.
Done @smoia
I am getting the attached failure: Let me know if there's something you can point me to that's wrong on my end! |
@m-miedema It seems that the code is the same version, and it passes locally I cannot recreate it. Also we cannot see the CI yet before physiopy/physutils#7 merges and releases Could you try to add the following to from loguru import logger
import sys
logger.add(sys.stderr) |
@m-miedema can't replicate the error you are getting either |
@maestroque Not sure if that should even be addressed in this PR, but in the chest_belt.py script, we are using the |
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 Thank for your work on that PR ! Good job overall 🎉 🎉
Would it be possible to add some tests to cover the CLI ? If you need some examples, you could refer to the tests in the giga_connectome package.
Yes, you are right! There is this open issue about numpy v2 compatibility opened for that #62 |
Sure, on it |
phys2denoise/workflow.py
Outdated
# def phys2denoise( | ||
# filename, | ||
# outdir=".", | ||
# metrics=[ | ||
# crf, | ||
# respiratory_pattern_variability, | ||
# respiratory_variance, | ||
# respiratory_variance_time, | ||
# rrf, | ||
# "retroicor_card", | ||
# "retroicor_resp", | ||
# ], | ||
# debug=False, | ||
# quiet=False, | ||
# **kwargs, | ||
# ): |
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.
Can we delete what is commented ?
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.
Oops, yeah I thought I did
This does not fix it - I'm not sure what's going on but since you and @me-pic are not able to replicate it, I continued on to test the rest of the CLI (see my next comment). |
I'm having trouble using the CLI to calculate metrics when using a .phys object as my input. For example, I thought I'd run a quick test using the physio objects generated in the OHBM tutorial here but the CLI returns e.g. "Metric rv not computed. Skipping" without more useful information (even when I run in --debug mode - which as a side-note, doesn't actually change the output for me). Has anyone else successfully output metrics in this case? |
@m-miedema I need you to provide the precise logs you get in order to understand the problem. I haven't had this issue personally |
Certainly! If you've been able to get the CLI to run on a Physio object with peaks/troughs, could you share the object and the call with me and I can try it? So far I've tried a few different ways, but in general this is the type of call and output:
|
One thing I think we should strongly consider as a future direction for the CLI is to set up a heuristic file with more metric specific parameters. For example, here we can calculate different metrics, but not specific different window sizes for each in the same call. I'm putting this comment along with a new issue here not to lose track of it - if others think this is a useful idea I can follow up in the future :) |
@maestroque I think it would be very helpful to make the "Metric rv not computed. Skipping" message slightly more verbose so that the user knows it is stemming from the export argument, rather than the computational argument. As it stands it's quite confusing! E.g. "Metric X not computed, skipping the export of metric X." or even better to throw a warning when a metric is provided as an export argument but not a computational argument. |
As well, I opened a new issue to address this, but I'm finding that the number of time points in the exported resampled metric files don't match the -nscans argument in the CLI. I think users would expect this to be the case, so it's something we should dig into another time. |
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 looking forward to seeing more documentation and to resolving some of the logging integration with pydra (I will open an issue if I'm still having logging-related failures in physutils and phys2denoise local testing following the merge). Please be sure to provide an example of running the CLI and the expected outputs in the documentation, including logs. Other than my minor point about the calculated vs. exported metric message, I won't suggest any other changes to address at this point. Thanks for all your hard work @maestroque !
Cleaned up, this should be ready to merge once physutils is |
@maestroque thanks for updating this one! |
Closes #
Proposed Changes
Change Type
bugfix
(+0.0.1)minor
(+0.1.0)major
(+1.0.0)refactoring
(no version update)test
(no version update)infrastructure
(no version update)documentation
(no version update)other
Checklist before review