FIX: NiBabel 5, and NetworkX 3 and DIPY 1.6 compatibility #3538
Conversation
NiBabel 4 began warning that int64 images would error, and NiBabel 5 began erroring if not passed an explicit dtype or header. We don't need int64 images, just set some sensible dtypes.
Codecov ReportBase: 63.63% // Head: 63.62% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3538 +/- ##
==========================================
- Coverage 63.63% 63.62% -0.02%
==========================================
Files 309 309
Lines 40873 40900 +27
Branches 5381 5381
==========================================
+ Hits 26011 26021 +10
- Misses 13838 13855 +17
Partials 1024 1024
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 at Codecov. |
|
Looks like dipy compatibility as well. @skoudoro anything we should know before digging in? I might have a chance this afternoon. |
|
Nothing particular change. Looking at the error, it seems that a new workflow has a unknown argument type which surprise me. I can look at this only tonight, quite busy today. Thank you for pinging @effigies |
|
Thanks. I'll let you know if I find anything. If it's just a bug, the easy fix here will be to blacklist the latest release of dipy. Then there shouldn't be a huge rush for you to release. |
|
Ok, I just looked deeper and you just need to add the type "str" in the function "convert_to_traits_type". This function is handling the keyword "string" but not "str". 😅 It should be an easy and fast fix of this function in nipype. Now, I see why I didn't catch it in DIPY. |
effigies
force-pushed
the
fix/nibabel5
branch
from
cee2924 to
4622fa9
Compare
| @@ -110,7 +110,7 @@ def convert_to_traits_type(dipy_type, is_file=False): | |||
| """Convert DIPY type to Traits type.""" | |||
| dipy_type = dipy_type.lower() | |||
| is_mandatory = bool("optional" not in dipy_type) | |||
| if "variable" in dipy_type and "string" in dipy_type: | |||
| if "variable" in dipy_type and "str" in dipy_type: | |||
There was a problem hiding this comment.
I tested this on 1.5.0 (which was not failing) and 1.6.0 (which was). Both pass.
effigies
force-pushed
the
fix/nibabel5
branch
from
4622fa9 to
a7295cf
Compare
effigies
force-pushed
the
fix/nibabel5
branch
from
c3a1909 to
f20035c
Compare
effigies
force-pushed
the
fix/nibabel5
branch
from
627d7cf to
f6bf0af
Compare
effigies
changed the title
There was a problem hiding this comment.
LGTM, thanks for the amazing work. Solves the issue I just opened and supersedes my networkx PR.
(I still have one error, but that is fully unrelated to dependency compatibility fixes)
Summary
This PR makes minimal changes to handle deprecations in nibabel 5 and networkx 3.
As seen in #3535, simply switching to sparse arrays breaks the dependency graph matrix representation. However, scipy sparse arrays are just sparse matrices with wrappers on top, so we can just get back to the matrix for now. While they claim they are deprecating the API, much of the API still works with and produces matrices, so I think we have time where we can continue doing what works.
In the long run, it would be nice to understand what is treating these representations differently, but that's beyond the time I have to devote to this. I would rather get things working before embarking on a refactor, especially if it's just a cheap type coercion.
Fixes #3537.
Fixes #3530.
Fixes #3539.
List of changes proposed in this PR (pull-request)
nx.to_scipy_sparse_matrix()tossp.lil_matrix(nx.to_scipy_sparse_array())"str"instead of"string"