cherab / core

The core source repository for the Cherab project.
https://www.cherab.info
Other
44 stars 24 forks source link

Rescale matrices and vectors in invert_regularised_nnls and allow kwargs. #439

Closed vsnever closed 3 months ago

vsnever commented 3 months ago

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.

jacklovell commented 3 months ago

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.

vsnever commented 3 months ago

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.