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

Fix bad phase connection when using --denoise-after-combining #781

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jul 12, 2024

Closes #775. In #679, we added support for complex-valued denoising, but only if --denoise-after-combining isn't used. This removes a connection from when I tried supporting complex-valued denoising when --denoise-after-combining is used.

It looks like this bug was found by running --denoise-after-combining. @mattcieslak any chance we could modify one of the tests to use --denoise-after-combining? Not sure if there are any test datasets with multiple runs.

Changes proposed in this pull request

  • Remove expected out_dwi_phase output from merge_dwis node.

@tsalo tsalo added the bug Something isn't working label Jul 12, 2024
@tsalo tsalo requested a review from mattcieslak July 12, 2024 14:57
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.77%. Comparing base (65791db) to head (9d0778c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #781   +/-   ##
=======================================
  Coverage   29.77%   29.77%           
=======================================
  Files          97       97           
  Lines       14583    14583           
  Branches     1886     1886           
=======================================
  Hits         4342     4342           
  Misses      10115    10115           
  Partials      126      126           

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

@mattcieslak
Copy link
Collaborator

I have always thought that --denoise-after-combining is a bad idea. It was originally included for pipeline testing as kind of a strawman. Maybe the best option would be to just remove it?

@mattcieslak
Copy link
Collaborator

also I don't think any of the test datasets have multiple runs :(

@tsalo
Copy link
Member Author

tsalo commented Jul 12, 2024

Unfortunately, I don't know enough about it to have an opinion about --denoise-after-combining. It looks like drbuddi_rpe_series has an AP and a PA run of the DWI. Would the parameter be applicable there?

@mattcieslak mattcieslak merged commit 7506edb into PennLINC:master Aug 9, 2024
32 checks passed
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.

Module merge_dwis has no output called out_dwi_phase
3 participants