Skip to content

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

Merged
jacklovell merged 4 commits intocherab:developmentfrom
vsnever:fix/nnls
Jun 3, 2024
Merged

Rescale matrices and vectors in invert_regularised_nnls and allow kwargs.#439
jacklovell merged 4 commits intocherab:developmentfrom
vsnever:fix/nnls

Conversation

@vsnever
Copy link
Member

@vsnever vsnever commented May 14, 2024

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 vsnever requested review from Mateasek and jacklovell May 14, 2024 16:35
Copy link
Member

@Mateasek Mateasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good code wise. Since I'm not familiar with tomographic inversions I can't judge if there are any other issues.

@jacklovell
Copy link
Member

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
Copy link
Member Author

vsnever commented May 23, 2024

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.

@jacklovell jacklovell merged commit b3cc6ad into cherab:development Jun 3, 2024
@vsnever vsnever deleted the fix/nnls branch August 30, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants