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

Avoid dipy versions 1.6.0 + 1.7.0 that contain a method=restore bug #4232

Merged

Conversation

joshuacwnewton
Copy link
Member

Description

sct_dmri_compute_dti fails on sct_example_data images when using -method restore:

sct_dmri_compute_dti -i dmri.nii.gz -bval bvals.txt -bvec bvecs.txt -method restore

I dug into this on the dipy side of things, and found that the issue only exists for dipy==1.6.0,1.7.0, and has already been fixed upstream. dipy==1.8.0 is releasing soon, so to avoid freezing a buggy version for SCT v6.1, we skip these two versions in our requirements.txt.

Note: We have an existing test for this. The reason that our test suite didn't catch this is because dipy chunks the processing, and the test image is smaller than a single chunk, but the bug only occurs when there are >1 chunks. I'm not quite sure how to test for this given our available testing data? But maybe given that this is a one-off issue with an upstream library, we don't necessarily need to test for this.

Linked issues

Fixes #4209.

See dipy/dipy#439 (comment) for a description of the problem, as well as a description of the fix that has already been implemented upstream.
@joshuacwnewton joshuacwnewton added bug category: fixes an error in the code sct_dmri_compute_dti context: labels Sep 28, 2023
@joshuacwnewton joshuacwnewton added this to the 6.1 milestone Sep 28, 2023
@joshuacwnewton joshuacwnewton self-assigned this Sep 28, 2023
requirements.txt Outdated Show resolved Hide resolved
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and the upstream report! And avoiding buggy versions (like you did) seems nicer than pinning.

@joshuacwnewton joshuacwnewton merged commit 8a49d29 into master Sep 28, 2023
24 checks passed
@joshuacwnewton joshuacwnewton deleted the jn/4209-avoid-buggy-dipy-versions-method-restore branch September 28, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category: fixes an error in the code sct_dmri_compute_dti context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError: could not broadcast array (7) into shape (12) during -method restore
2 participants