Skip to content

Implementation of the workflow#11

Merged
goodalse2019 merged 13 commits intophysiopy:masterfrom
celprov:fix/debugging
Jun 20, 2024
Merged

Implementation of the workflow#11
goodalse2019 merged 13 commits intophysiopy:masterfrom
celprov:fix/debugging

Conversation

@celprov
Copy link
Copy Markdown
Collaborator

@celprov celprov commented Jul 21, 2023

Closes #

This PR implements the workflow.

Proposed Changes

  • Implement a parser (strongly inspired by the parser of phys2denoise)
  • Add an interface to iteratively generates the figures
  • Add an interface to compute the metrics iteratively
  • Add an interface to save the metrics as a json or csv
  • Adapt the documentation to the reality that certain metrics function can take as input a peakdet Physio object or a np.array
  • Remove smoothness as its implementation is not working

!! The generation of the visual report is missing in the workflow and needs to be added !!

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

@SRSteinkamp
Copy link
Copy Markdown
Collaborator

@celprov

In smoothness, you use:

    time = np.arange(0, len(data.data) / data.fs, 1 / data.fs)
    dx2 = np.empty(len(time))
    for t in time:
        dx2[t] = derivative(data.data, t, n=2)

for the derivative, could we simply replace it with np.diff(np.diff(data.data)), i.e. computing the derivative numerically, assuming a dt of 1?

@celprov
Copy link
Copy Markdown
Collaborator Author

celprov commented Dec 4, 2023

@SRSteinkamp oula it has been a long time ^^'
Given that I left a comment that this function currently did not work, I guess any alternative implementation is welcome. So I guess it should be fine.

@SRSteinkamp
Copy link
Copy Markdown
Collaborator

SRSteinkamp commented Dec 4, 2023

Jup, it has been long ;) ... I'm not sure anymore if it is as simple as taking the two np.diffs going to think about it.

Quick update (@celprov , @smoia ):

I think

time = np.arange(0, len(data.data) / data.fs, 1 / data.fs)
dx1 = np.gradient(data.data, time)
dx2 = np.gradient(dx1, time)

might be the way to go, as it keeps the unit of the derivative at (hope I have the notation right): d^2 x / dt^2. Is it necessary to further normalize it? - i.e. if we want to compare signals with different sampling rates?

There are still massive differences in terms of the mean absolute value for the time series, dependent on the sampling rate. Which makes sense, if we redefine the smoothness measure as "ruggedness" or something, i.e. the higher the value, the less smooth the signal.

@smoia
Copy link
Copy Markdown
Member

smoia commented Dec 5, 2023

I'm trying to wrap my head around this, but I don't have enough signal processing background to provide any useful suggestion.

However, a couple of things:

  • does MRIQC compute smoothness? If so, it might be nice to check how it does it
  • in any case, I'd write in the docstrings what you mean by smoothness and why you decided to go for this implementation - it will make it easier for documentation to be clear and for users to understand how to interpret this measure (especially if, as you said, this measure is not so well defined)

@SRSteinkamp
Copy link
Copy Markdown
Collaborator

Same...
The only thing I have to go by is the GoogleDoc you shared for physioQC and Celine's implementation (which is not working as intended) ... so I am also not sure what smoothness is here and had hoped you had some insight :) ... also happy to remove the measure for now until we have a better grasp of it.

@celprov
Copy link
Copy Markdown
Collaborator Author

celprov commented Dec 5, 2023

@SRSteinkamp MRIQC computes the smoothness of the fMRI image as Full-width half maximum smoothness (fwhm*)
Here is the info I've found in MRIQC code about how it is implemented

fwhm (nipype interface to AFNI): The :abbr:FWHM (full-width half maximum) of
the spatial distribution of the image intensity values in units of voxels [Forman1995]_.
Lower values are better, higher values indicate a blurrier image. Uses the gaussian
width estimator filter implemented in AFNI's 3dFWHMx:

.. math ::

  \text{FWHM} = \sqrt{-{\left[4 \ln{(1-\frac{\sigma^2_{X^m_{i+1,j}-X^m_{i,j}}}
  {2\sigma^2_{X^m_{i,j}}}})\right]}^{-1}}

Here is the visual representation of the equation
image

@smoia smoia requested a review from m-miedema December 5, 2023 20:58
@celprov celprov mentioned this pull request Dec 5, 2023
18 tasks
@celprov celprov changed the title Fix bugs Implementation of the workflow Dec 5, 2023
@SRSteinkamp
Copy link
Copy Markdown
Collaborator

Will have a look if it fits for 1D time series signals (and not images) @celprov :)

@smoia ... before this PR is ready for review, we need to review and merge #5 first. This PR already references the visualizations we implemented.

@celprov
Copy link
Copy Markdown
Collaborator Author

celprov commented Dec 5, 2023

In the project wrap-up, we proposed to separate the smoothness from this PR, so that the rest of the content can already be reviewed and merged and to work on developing the smoothness in another PR.

@SRSteinkamp feel free to open a draft PR with your implementation of the smoothness.

@celprov
Copy link
Copy Markdown
Collaborator Author

celprov commented Dec 5, 2023

@m-miedema I think this PR is ready for review (unless @SRSteinkamp says otherwise).

@SRSteinkamp
Copy link
Copy Markdown
Collaborator

SRSteinkamp commented Dec 6, 2023

Couldn't check if the actual implementation of the workflow yet, as visualizations are not included.

Please prioritize #5 - after #5 is merged in main (and this here) I will finish what is left to do on this PR.

@smoia smoia added the Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) label Apr 18, 2024
@m-miedema
Copy link
Copy Markdown
Member

m-miedema commented Apr 18, 2024

This is a really exciting PR! I know @SRSteinkamp still wants to work on some visualizations but I thought I'd go ahead and take a look in the meantime. However, I can't get the CLI to run. I'm on Windows (I know this is new ground) and after installing with pip, and running physioqc -h I receive the following error:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code 
  File "C:\Users\data_acq\AppData\Local\Programs\Python\Python311\Scripts\physioqc.exe\__main__.py", line 4, in <module>
  File "C:\Users\data_acq\AppData\Local\Programs\Python\Python311\Lib\site-packages\physioqc\workflow.py", line 10, in <module>
    from physioqc.cli.run import _get_parser
ModuleNotFoundError: No module named 'physioqc.cli'

I haven't figured out how to solve this myself, although @me-pic has replicated the error running on a Mac.

@m-miedema
Copy link
Copy Markdown
Member

m-miedema commented Apr 18, 2024

It seems like there were some __init__ files missing! Thank you @me-pic :)
After that, it seems like the main thing is to add in the visualizations. @SRSteinkamp let me know if you need any help :)

@smoia
Copy link
Copy Markdown
Member

smoia commented Apr 18, 2024

@m-miedema or @me-pic can you push the missing init to this branch?

@SRSteinkamp SRSteinkamp mentioned this pull request Apr 19, 2024
18 tasks
@celprov
Copy link
Copy Markdown
Collaborator Author

celprov commented Apr 19, 2024 via email

@m-miedema
Copy link
Copy Markdown
Member

@celprov I haven't been able to fully test it yet, as the workflow file was raising errors where the visualizations are missing.

@celprov
Copy link
Copy Markdown
Collaborator Author

celprov commented Apr 19, 2024

Is it a matter that the visualizations missing are implemented in PR #5 ?

@m-miedema
Copy link
Copy Markdown
Member

I'm not quite sure -- from what I can tell, the visualizations have inconsistent naming conventions across the two branches, but it seems like from @SRSteinkamp's comment above, it will not be too difficult to implement once that PR has been merged.

Copy link
Copy Markdown
Collaborator

@goodalse2019 goodalse2019 left a comment

Choose a reason for hiding this comment

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

Looks good from my end!

Copy link
Copy Markdown
Collaborator

@goodalse2019 goodalse2019 left a comment

Choose a reason for hiding this comment

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

Try 2, looks good

Copy link
Copy Markdown
Collaborator

@goodalse2019 goodalse2019 left a comment

Choose a reason for hiding this comment

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

yes!

Copy link
Copy Markdown
Collaborator

@goodalse2019 goodalse2019 left a comment

Choose a reason for hiding this comment

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

I officially viewed!

@smoia smoia requested a review from goodalse2019 June 20, 2024 05:43
Copy link
Copy Markdown
Collaborator

@goodalse2019 goodalse2019 left a comment

Choose a reason for hiding this comment

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

looks good!

@goodalse2019 goodalse2019 merged commit db69f1f into physiopy:master Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0)

Projects

Archived in project
Status: PR review in progress

Development

Successfully merging this pull request may close these issues.

5 participants