Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

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.

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.

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.

None yet

3 participants