I do not see a way to change step as used by reconst.dti.TensorModel.fit()

[captainnova]

Description

DTI fitting seems to have gotten slower between dipy 1.6.0 and 1.8.0. Raising the value of the optional step parameter in iter_fit_tensor() is suggested, e.g. in https://docs.dipy.org/stable/reference/dipy.reconst.html#dipy.reconst.dti.TensorModel , but if I change

dtm = dti.TensorModel(gtab, fit_method=fit_meth, **fit_kwargs)
dt = dtm.fit(data, mask)

to

dtm = dti.TensorModel(gtab, fit_method=fit_meth, step=1e5, **fit_kwargs)
dt = dtm.fit(data, mask)

I get TypeError: nlls_fit_tensor() got an unexpected keyword argument 'step'.

Stepping through the code, it appears that specifying step in either TensorModel.__init__ or iter_fit_tensor() would be fine, but the intermediate layers to abstract the kind of fit method do not accept the step parameter. nlls_fit_tensor() even calls ols_fit_tensor() internally without the step parameter, so that would need to be caught too.

I understand how step tries to find a happy medium between voxel-by-voxel looping and whole-image-out-of-memory problems, but what is the right target for the amount of memory? The CPU RAM or the GPU memory?

Am I missing something?

Thanks

[skoudoro]

Hi @captainnova,

DTI fitting seems to have gotten slower between dipy 1.6.0 and 1.8.0.

Strange! How much slower? Do you have some comparaison ? Which OS are you using?
Thank you for the feedback, in the todo, I will look into it, we would like to avoid this kind of regression.

Stepping through the code, it appears that specifying step in either....

Seems to work with 'wls' or 'ols', maybe the decorator is just missing for nlls or there is a specific reason to that. I need to dig a bit or maybe @arokem and @Garyfallidis know the reason

I understand how step tries to find a happy medium between voxel-by-voxel looping and whole-image-out-of-memory problems, but what is the right target for the amount of memory? The CPU RAM or the GPU memory?

I let @arokem or @Garyfallidis answer this.

Thank you for the feedback, and I will look this month at the speed issue.

[captainnova]

Hi @skoudoro,

I apologize; further testing showed that the apparent slowdown only happens while running in a debugger. I was doing that because of the dti bug in 1.7.0, and then kept using it in 1.8.0 so I could see what was happening. I did not have a breakpoint inside .fit() itself, but it was a bad environment for estimating speed from. It was however useful to see the disconnect between step=... at the iter_fit_tensor() level and the higher level TensorModel.fit().

When I wrote a test script to isolate fitting the difference essentially disappeared:

$ python ../time_nnls.py 
dipy version: 1.8.0
Best wall-clock time of 3: 43.7444 s
Best CPU time of 3: 43.6054 s

$ /usr/bin/python ../time_nnls.py
dipy version: 0.16.0
Best wall-clock time of 3: 41.727 s

Ideally I would have compared python 1.6.0 and 1.8.0, but the handy py3 virtualenvs were dipy 1.7.0 and 1.8.0, and 1.7.0 was broken. 0.16.0 goes back to python 2, but I don't think much changed except the stepping.

So, the practical importance of this issue just dropped to almost nothing, but the documentation is a bit confusing since it suggests changing step, and that is not currently an option of NNLS.

The timing script preloaded the data and mask, so the time was just instantiating a TensorModel() and running .fit(). The shape of the masked data being fit was (181669, 127).

[arokem]

The "step" parameter cannot be used in NNLS optimization in the same way that it's being used in WLS/OLS.

I think that we can introduce some logic where if the user selected NNLS and passed a step that keyword argument gets ignored (and removed from the fit_kwargs in the init), so that users get a uniform outward facing API.

[captainnova]

I think that we can introduce some logic where if the user selected NNLS and passed a step that keyword argument gets ignored (and removed from the fit_kwargs in the init), so that users get a uniform outward facing API.

Maybe it would be simpler to add an optional step parameter to nlls_fit_tensor() and note in the docstring that it is not used?

[arokem]

Maybe it would be simpler to add an optional step parameter to nlls_fit_tensor() and note in the docstring that it is not used?

Yeah - that should work. Do you want to make a PR with that? I'd start by making a test that fails with the current implementation (i.e., raises the error that you mentioned in your OP), and then add the fix.