Skip to content

Conversation

@tsalo
Copy link
Member

@tsalo tsalo commented May 28, 2025

Closes #550.

Changes proposed in this pull request

  • Generate and write out a new transform, <source_entities>_from-m0scan_to-aslref_mode-image_xfm.txt, to register a separate M0 scan to the ASL reference image. If the separate M0 scan is used as the reference image, then this will be an identity transform.
    • What if there are multiple M0 volumes in the separate M0 scan file, the separate M0 scan is used as the reference image, and there's motion? We should probably run MCFLIRT to the first volume and write that out as the transform, right?

@tsalo tsalo added the bug Something isn't working label May 28, 2025
@tsalo
Copy link
Member Author

tsalo commented May 29, 2025

Somehow this PR fully ruins the coregistration, brain masking, etc. for the Philips test data. Siemens, PCASL multi-PLD, and test_002 (GE) look fine.

Before

before

After

after

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2025

Codecov Report

Attention: Patch coverage is 76.85950% with 28 lines in your changes missing coverage. Please review.

Project coverage is 76.06%. Comparing base (4ece8ec) to head (343e5cc).

Files with missing lines Patch % Lines
aslprep/interfaces/utility.py 61.01% 23 Missing ⚠️
aslprep/workflows/asl/fit.py 89.36% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
- Coverage   76.08%   76.06%   -0.03%     
==========================================
  Files          38       38              
  Lines        4106     4219     +113     
==========================================
+ Hits         3124     3209      +85     
- Misses        982     1010      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsalo
Copy link
Member Author

tsalo commented May 29, 2025

The aslref image looks completely different too, so it must be due to the changes I made in the aslref workflow.

@tsalo tsalo requested a review from mattcieslak May 29, 2025 20:33
@tsalo
Copy link
Member Author

tsalo commented May 29, 2025

Okay, I think I've fixed all of the issues. @mattcieslak would you be willing to review this at some point?

Copy link
Contributor

@mattcieslak mattcieslak left a comment

Choose a reason for hiding this comment

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

I don't see any problems here, but it does remind me of an issue that Phil and Chris were having where they could only successfully coregister the ASL data to the structural scans if they used a cbf image. I think some of the logic you implemented here might also be used to address that problem in a future PR

@tsalo tsalo merged commit e464b62 into main May 29, 2025
26 checks passed
@tsalo tsalo deleted the m0scan-transform branch May 29, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate M0 scan with different resolution from ASL will raise error

4 participants