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

Caching of the LU decomposition computed by _InverseLinearOperator #737

Conversation

marvinpfoertner
Copy link
Collaborator

@marvinpfoertner marvinpfoertner commented Nov 8, 2022

In a Nutshell

Currently, executing

A = pn.linops.Matrix(...)
x1 = A.inv() @ b1
x2 = A.inv() @ b2

runs scipy.linalg.lu_factor on A.dense twice.
In principle, it would be possible to cache the return value of A.inv(), but that would introduce cyclic references,
which might cause memory overhead before being collected by the garbage collector.
An elegant solution is to cache all expensive quantities in the linear operator A itself, and then create "throwaway" objects like _InverseLinearOperator and TransposeLinearOperator, which are essentially views of A, accessing these quantities through the base linear operator.

Detailed Description

  • caching of LU decomposition in LinearOperators
  • bugfix in _InverseLinearOperator.__rmatmul__
  • add test case for _InverseLinearOperator
  • Replace custom implementation of LinearOperator.broadcast_matmul with np.vectorize-based implementation

Related Issues

None

@marvinpfoertner marvinpfoertner added bug Something isn't working refactoring Refactoring of existing functionality improvement Improvements of existing functionality linops Issues related to linear operators labels Nov 8, 2022
@marvinpfoertner marvinpfoertner self-assigned this Nov 8, 2022
@marvinpfoertner marvinpfoertner requested a review from a team as a code owner November 8, 2022 17:00
@marvinpfoertner
Copy link
Collaborator Author

marvinpfoertner commented Nov 8, 2022

@JonathanWenger This PR should be reviewed after #735 is merged, since it branches off of it.

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #737 (f206691) into main (ce4bbba) will decrease coverage by 0.04%.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #737      +/-   ##
==========================================
- Coverage   90.40%   90.36%   -0.05%     
==========================================
  Files         197      197              
  Lines        7454     7431      -23     
  Branches      966      964       -2     
==========================================
- Hits         6739     6715      -24     
- Misses        481      482       +1     
  Partials      234      234              
Impacted Files Coverage Δ
src/probnum/linops/_linear_operator.py 87.38% <79.16%> (-0.67%) ⬇️

Copy link
Contributor

@JonathanWenger JonathanWenger left a comment

Choose a reason for hiding this comment

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

Nice! Elegant solution of the circular dependency issue.

src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
Returns
-------
sol
The solutions :math:`A^{-1} X` of the linear systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The solutions :math:`A^{-1} X` of the linear systems.
The solution(s) :math:`A^{-1} X` of the linear system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually always have multiple solutions here, since this method only works if x.ndim == 2.
I added an appropriate assertion.

@marvinpfoertner marvinpfoertner merged commit 6fadc00 into probabilistic-numerics:main Nov 9, 2022
@marvinpfoertner marvinpfoertner deleted the inverse-linop-caching branch November 9, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working improvement Improvements of existing functionality linops Issues related to linear operators refactoring Refactoring of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants