Rescale matrices and vectors in invert_regularised_nnls and allow kwargs.#439
Rescale matrices and vectors in invert_regularised_nnls and allow kwargs.#439jacklovell merged 4 commits intocherab:developmentfrom
Conversation
Mateasek
left a comment
There was a problem hiding this comment.
This seems good code wise. Since I'm not familiar with tomographic inversions I can't judge if there are any other issues.
|
Thanks for this @vsnever: we're still stuck on Python 3.7 here at Culham and so don't get to experience these Scipy "improvements" often. Accuracy and performance regressions in a library as well maintained as Scipy are concerning: I'd have expected better backwards compatibility unless we're somehow misusing the Scipy functions in Cherab[*]. Have you checked that this normalisation scheme gives identical results (at least to within numerical precision) as the original function, for Scipy < 1.12.0? Intuitively I feel like it should, but it'd be nice to get confirmation. I don't think we have any unit tests to check for regression in the matrix inversion tools. [*] I'm beginning to think scipy.optimize.nnls is not the best routine to use for bolometry actually: since both the geometry matrix and the regularisation operator are sparse we could likely benefit from using some of Scipy's sparse-aware routines. I'll make a separate issue and take a look into this. |
Yes, I tested this. For older versions of SciPy, the results with and without normalisation are identical. However, I forgot to change the |
This fixes #438 by rescaling the
w_matrix,b_vectorandtikhonov_matrixbefore passing them tonnlsto avoid possible issues with the recent versions of SciPy. Also,**kwargsare added, so the user can control thennlstermination criteria.