core icon indicating copy to clipboard operation
core copied to clipboard

Rescale matrices and vectors in invert_regularised_nnls and allow kwargs.

Open vsnever opened this issue 1 year ago • 1 comments

This fixes #438 by rescaling the w_matrix, b_vector and tikhonov_matrix before passing them to nnls to avoid possible issues with the recent versions of SciPy. Also, **kwargs are added, so the user can control the nnls termination criteria.

vsnever avatar May 14 '24 16:05 vsnever

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.

jacklovell avatar May 20 '24 13:05 jacklovell

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.

Yes, I tested this. For older versions of SciPy, the results with and without normalisation are identical. However, I forgot to change the rnorm scale back. Now fixed.

vsnever avatar May 23 '24 23:05 vsnever