Skip to content
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

Drop AFNI dependency #19

Merged
merged 38 commits into from
Oct 13, 2022
Merged

Drop AFNI dependency #19

merged 38 commits into from
Oct 13, 2022

Conversation

eurunuela
Copy link
Collaborator

@eurunuela eurunuela commented Jun 22, 2022

Closes #18.

Changes proposed in this pull request:

  • hrf_generator.py uses nilearn's glm.first_level module to generate the HRF matrix.
  • hrf_generator.py returns the normalized HRF as self.hrf_ instead of returning a non-normalized and a normalized HRF.
  • hrf_generator.py can read a user defined HRF from a 1D file.
  • The pySPFM command is added to the header of the output files with nibabel.

Edit: The support for adding the command to the header is dropped as part of this PR until nibabel supports it.

Edit 2: This PR also renames nscans and nvoxels to n_scans and n_voxels.

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #19 (6d58475) into main (cb38b5f) will decrease coverage by 2.51%.
The diff coverage is 80.64%.

@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
- Coverage   86.24%   83.73%   -2.52%     
==========================================
  Files           9        9              
  Lines         647      670      +23     
==========================================
+ Hits          558      561       +3     
- Misses         89      109      +20     
Impacted Files Coverage Δ
pySPFM/deconvolution/hrf_generator.py 68.96% <64.00%> (-29.76%) ⬇️
pySPFM/workflows/pySPFM.py 90.90% <88.00%> (-0.48%) ⬇️
pySPFM/io.py 93.75% <100.00%> (+5.28%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pySPFM/cli/run.py Outdated Show resolved Hide resolved
This is a big commit that adds BIDS compatibility assuming the user provides the correct prefix (i.e, pySPFM only adds BIDS-compatible suffix and sidecar).

This commit also adds support for custom HRFs.

This commit updates the integration tests to adapt to these changes and test the BIDS outputs.
@eurunuela
Copy link
Collaborator Author

@tsalo what do you think of the changes I made?

Do you think it's reasonable that pySPFM only makes sure that the suffix is BIDS-compatible?

Copy link
Contributor

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Sorry, I only saw your comment after I got notifications from your recent commits. I think there are a lot of novel outputs (i.e., ones not covered by BIDS or current BIDS extensions), but we can probably make them at least fairly BIDS-like. I don't understand the outputs enough to make good recommendations at the moment, but once there's a documentation file outlining the outputs I might be able to help.

I don't think pySPFM has to take responsibility for the incoming filenames (e.g., the sub entity), but I would like to include other derivative entities, like desc and/or stat, in the output filenames, like we do in tedana.

pySPFM/workflows/pySPFM.py Outdated Show resolved Hide resolved
pySPFM/workflows/pySPFM.py Outdated Show resolved Hide resolved
Comment on lines 607 to 608
output_name = get_outname(output_filename, "innovation", "nii.gz", is_bids)
out_bids_keywords.append("innovation")
Copy link
Contributor

Choose a reason for hiding this comment

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

innovation isn't a supported suffix in BIDS. What is the innovation file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The innovation file contains the estimates of the innovation signal; i.e., the derivative of the activity-inducing signal (which in the case of multi-echo reflects the changes in R2*).

Comment on lines 621 to 622
output_name = get_outname(output_filename, "beta", "nii.gz", is_bids)
out_bids_keywords.append("beta")
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a parameter estimate, then stat-effect_statmap might be the way to go. I don't think there's any recommendation for parameter estimate time series (e.g., beta series analyses), but it's something I can bring up in the associated BIDS extension proposal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to come up with a better name for the activity-inducing (beta) and activity-related (fitted, denoised BOLD) files in single-echo analyses.

We have been using the beta keyword because you can think of SPFM as a "blind GLM" that estimates the activity maps at each TR with no information about the timings of the neuronal events. Hence, the use of beta.

I would like to make it more explicit and use activity-inducing and activity-related or bold-denoised, but I don't know how that would work with BIDS. Do you have any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

desc-denoised_bold is good for the latter... I'll have to think a bit about the former. Something like contrast-none_stat-effect_statmap might be good?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like desc-denoised_bold but I don't know about contrast-none_stat-effect_statmap. I honestly would not understand what that stands for 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

In fitlins/my BEP, the contrast keyword describes the contrast (e.g., faces > houses might be facesMinusHouses), the stat keyword indicates what type of value is in the file (e.g., effect for beta/contrast values, variance for variance values, z for z statistics, etc.), and statmap is the standard suffix for statistical maps, like bold is for fMRI data before any kind of convolution.

What about contrast-activityInducing_stat-effect_statmap and contrast-activityRelated_stat-effect_statmap?

Copy link
Contributor

Choose a reason for hiding this comment

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

Argh, this is so hard! I will say that "stat-effect" means that it's the beta value from a regression, rather than that it's a meaningful statistic, but I see your point.

I guess desc-activityInducing_bold would be the most interpretable, but desc-innovation works too, I guess? My problem is that I'm just not familiar with innovation signal as a term in neuroimaging- it mostly makes me think of "new and fancy methods" or the tech industry. Still, that's not a good reason to avoid the term if it's established, so you should use whichever desc makes more sense to you, but with the bold suffix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem with the bold suffix is that these signals are not BOLD signals, but what drive the BOLD signal. I know it's not easy haha.

In our terminology, BOLD = activity-related signal, and then we have the activity-inducing signal (which induces the BOLD response), and the innovation signal (which is the derivative of the activity-inducing signal).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I finally understand what the problem is, thank you! So basically BIDS doesn't yet have a suffix for the "neural" signal estimated by deconvolving the BOLD signal. We definitely need one, since there are a lot of methods for estimating this beyond SPFM. The problem is that everything I can think of is too general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry if I didn't make it clear until now 😅

I will go with desc-activityInducing and desc-innovation then, until BIDS comes up with a suffix for this kind of data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By the way, I just realized I made a mistake when I said the the $\Delta R_2^$ changes are in percent signal change. This is not correct. The denoised BOLD files are, but $\Delta R_2^$ changes are in $s^{-1}$.

So I think desc-activityInducing adding the units makes sense.

Comment on lines 624 to 625
output_name = get_outname(output_filename, "DR2", "nii.gz", is_bids)
out_bids_keywords.append("DR2")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the scale of this file the actual R2* values, or just the fluctuations? If it's a time series of R2* values, then I think R2starmap.nii.gz might be the way to go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a time series of the estimate of changes in R2* that drive the BOLD response.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but if R2* starts at 1 / 30 (i.e., T2* is 30) for one of the voxels, will this file have a value of 1 / 30, or 0 (since it's the starting point)?

Copy link
Collaborator Author

@eurunuela eurunuela Jul 27, 2022

Choose a reason for hiding this comment

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

Sorry. It will be in percent signal change, as the input to pySPFM has to be in percent signal change. So the first value will be 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about it a bit, I think something like desc-percentSignalChange_R2starw would make sense, since it's not in R2* scale.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be nice to include Units in the associated JSON, with a value of "percent".

pySPFM/workflows/pySPFM.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I just have a few more questions/thoughts.

pySPFM/io.py Outdated Show resolved Hide resolved
pySPFM/utils.py Outdated Show resolved Hide resolved
Comment on lines 624 to 625
output_name = get_outname(output_filename, "DR2", "nii.gz", is_bids)
out_bids_keywords.append("DR2")
Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about it a bit, I think something like desc-percentSignalChange_R2starw would make sense, since it's not in R2* scale.

Comment on lines 621 to 622
output_name = get_outname(output_filename, "beta", "nii.gz", is_bids)
out_bids_keywords.append("beta")
Copy link
Contributor

Choose a reason for hiding this comment

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

In fitlins/my BEP, the contrast keyword describes the contrast (e.g., faces > houses might be facesMinusHouses), the stat keyword indicates what type of value is in the file (e.g., effect for beta/contrast values, variance for variance values, z for z statistics, etc.), and statmap is the standard suffix for statistical maps, like bold is for fMRI data before any kind of convolution.

What about contrast-activityInducing_stat-effect_statmap and contrast-activityRelated_stat-effect_statmap?

pySPFM/tests/data/nih_five_echo_outputs_verbose.txt Outdated Show resolved Hide resolved
Comment on lines 11 to 15
test-me_desc-pySPFM_echo-2_MAD.nii.gz
test-me_desc-pySPFM_echo-3_MAD.nii.gz
test-me_desc-pySPFM_echo-4_MAD.nii.gz
test-me_desc-pySPFM_echo-5_MAD.nii.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the description in get_keyword_description, it seems like these are 3D images. Are they analogous to any outputs of other tools? For example, 3dREMLfit outputs "standard deviation of prewhitened residuals", which seems sort of similar, while SPM outputs a ResMS.img with the variance of the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, they show have a name similar to the outputs of 3dREMLfit or ResMS.img.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are they actually in the same units as either one though? The output of 3dREMLfit is labeled with the residwhstd suffix in fitlins, so using that directly would be awesome, if it's accurate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about the units on this one.

The MAD represents the pre-estimated noise level, which is derived from the median absolute deviation of fine-scale wavelet coefficients (Daubechies, order 3).

pySPFM/tests/data/nih_five_echo_outputs_verbose.txt Outdated Show resolved Hide resolved
@eurunuela
Copy link
Collaborator Author

eurunuela commented Oct 13, 2022

I believe this is ready to be merged. If anything, I can add better support for BIDS in a following PR.

Edit: for the record, I had to fix the version of nilearn to 0.9.1. Once the glm submodule is more stable I can let the dependency have a higher version.

@eurunuela eurunuela merged commit 76e67cd into main Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove non-Python dependencies
2 participants