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

Use correct bolus values in CBF calculation and temporarily disable Q2TIPS, multi-PLD, and no-BolusCutOff processing #235

Merged
merged 70 commits into from
Mar 29, 2023

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Mar 15, 2023

Closes #167.

Changes proposed in this pull request

  • For (P)CASL data, use LabelingDuration as the bolus value in BASIL CBF calculation. For PASL data, use BolusCutOffDelayTime instead.
  • Apply the same fix to the vanilla CBF calculation.
  • Raise an error if PASL data do not have BolusCutOffFlag set to True.
  • Hopefully support QUIPSS PASL data as well.
  • Explicitly block multi-PLD and Q2TIPS data for now.

I still need to figure out what to do if BolusCutOffFlag is False, as well as if BolusCutOffTechnique is QUIPSS or Q2TIPS.

@tsalo tsalo added the bug Something isn't working label Mar 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2023

Codecov Report

Patch coverage: 63.58% and project coverage change: +0.49 🎉

Comparison is base (b1a516b) 45.09% compared to head (7b63aec) 45.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   45.09%   45.59%   +0.49%     
==========================================
  Files         129      132       +3     
  Lines       12302    12461     +159     
==========================================
+ Hits         5548     5682     +134     
- Misses       6754     6779      +25     
Impacted Files Coverage Δ
aslprep/interfaces/ge.py 24.35% <0.00%> (-1.26%) ⬇️
aslprep/workflows/base.py 88.60% <ø> (ø)
aslprep/tests/test_cli.py 72.58% <10.52%> (-27.42%) ⬇️
aslprep/workflows/asl/cbf.py 59.17% <36.25%> (-0.43%) ⬇️
aslprep/workflows/asl/gecbf.py 14.65% <42.85%> (+1.01%) ⬆️
aslprep/workflows/asl/qc.py 54.38% <54.38%> (ø)
aslprep/interfaces/cbf_computation.py 69.40% <62.43%> (-2.56%) ⬇️
aslprep/workflows/asl/plotting.py 65.45% <65.45%> (ø)
aslprep/utils/misc.py 66.27% <77.77%> (+2.72%) ⬆️
aslprep/tests/test_interfaces_cbf_computation.py 100.00% <100.00%> (ø)
... and 1 more

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrewrosss
Copy link
Contributor

andrewrosss commented Mar 16, 2023

I'm not a core team member, but I'm the one who opened #167. These changes seem reasonable to me, specifically, using LabelingDuration for (P)CASL and BolusCutOffDelayTime for PASL seems to adhere to the intent of the BIDS spec and would fix address the original issue.

aslprep/utils/misc.py Outdated Show resolved Hide resolved
@tsalo
Copy link
Member Author

tsalo commented Mar 16, 2023

Thank you for reviewing @andrewrosss. Your input is very much appreciated.

@andrewrosss
Copy link
Contributor

Happy to help! :) Thanks for the fix!

Nilearn's maskers add a singleton time dimension when the data are purely spatial.
Starting in 0.12.0, nilearn masker.transform on a 3D image will return a 1D array.
@tsalo tsalo added the breaking-change PRs that change results or interfaces. label Mar 29, 2023
@tsalo tsalo changed the title Fix PASL bug Use correct bolus values in CBF calculation and temporarily disable Q2TIPS, mulit-PLD, and no-BolusCutOff processing Mar 29, 2023
@tsalo tsalo merged commit 141cc83 into PennLINC:main Mar 29, 2023
@tsalo tsalo deleted the pasl-bug branch March 29, 2023 15:06
This was referenced Apr 4, 2023
@tsalo tsalo changed the title Use correct bolus values in CBF calculation and temporarily disable Q2TIPS, mulit-PLD, and no-BolusCutOff processing Use correct bolus values in CBF calculation and temporarily disable Q2TIPS, multi-PLD, and no-BolusCutOff processing Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change PRs that change results or interfaces. bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LabelingDuration not defined for PASL acquisitions
3 participants