[ENH] Update to dipy 1.13 - DO NOT MERGE -#1317
[ENH] Update to dipy 1.13 - DO NOT MERGE -#1317arnaudbore wants to merge 11 commits intoscilus:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1317 +/- ##
==========================================
- Coverage 72.63% 72.63% -0.01%
==========================================
Files 295 295
Lines 25588 25596 +8
Branches 3597 3601 +4
==========================================
+ Hits 18587 18591 +4
- Misses 5489 5490 +1
- Partials 1512 1515 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
CHrlS98
left a comment
CHrlS98
left a comment
There was a problem hiding this comment.
Pretty simple PR it'd seem. I did not try running the code but read carefully. Comments are mostly to make sure we really understand what is going on.
| 'fa.nii.gz') | ||
| ret = script_runner.run(['scil_tractogram_convert', in_fib, | ||
| 'gyri_fanning.trk', '--reference', in_fa]) | ||
| ret = script_runner.run(['scil_tractogram_convert', in_trk, |
There was a problem hiding this comment.
I take it we can't convert fib to trk anymore?
There was a problem hiding this comment.
We can but the fib file we have for our test are not correct.
There was a problem hiding this comment.
Should we update the file then, before merging the PR?
There was a problem hiding this comment.
@frheault do you think it's worth it ? we can remove the file or I create a new/fixed .fib file by converting the trk to fib.
| if vol_data.ndim == 4 and np.ndim(sigma) == 3: | ||
| data_denoised = np.empty_like(vol_data) | ||
| for vol_idx in range(vol_data.shape[-1]): | ||
| data_denoised[..., vol_idx] = nlmeans( | ||
| vol_data[..., vol_idx], sigma, | ||
| mask=mask_denoise, rician=not args.gaussian, | ||
| num_threads=args.nbr_processes) | ||
| else: | ||
| data_denoised = nlmeans( | ||
| vol_data, sigma, mask=mask_denoise, rician=not args.gaussian, | ||
| num_threads=args.nbr_processes) |
There was a problem hiding this comment.
this change would be because nlmeans does not support 4D volumes anymore?
| monkeypatch.chdir(os.path.expanduser(tmp_dir.name)) | ||
| in_fib = os.path.join(SCILPY_HOME, 'surface_vtk_fib', | ||
| 'gyri_fanning.fib') | ||
| in_trk = os.path.join(SCILPY_HOME, 'surface_vtk_fib', |
There was a problem hiding this comment.
not sure the name of this test really fits the content? What is surface_vtk_fib?
There was a problem hiding this comment.
"surface_vtk_fib" is the test folder used here.
frheault
left a comment
frheault
left a comment
There was a problem hiding this comment.
LGTM, I tested the filtering with ROI, density mapping and the cutting script because it seems like an important part of the PR and it is working well.
I think it would be a good moment to adress the ton of warnings related to '' UserWarning: Pass ['sh_order_max', 'basis_type'] as keyword args. From version 2.0.0 passing these as positional''
UserWarning: Pass ['bbox_valid_check'] as keyword args. From version 2.0.0 passing these as positional arguments will result in an error.
It is annoying when running tests, but also scary for using for no reason, this is easily solved. I think this should be done before next release (2.3)
arnaudbore
changed the title
3 participants
Quick description
Please include a summary of the changes and the related issue(s) or improvement(s).
Please also include relevant motivation and context. List any dependencies that are required for this change if needed.
...
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist