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

Refactor LinearOperator for method overriding #735

Merged

Conversation

timweiland
Copy link
Collaborator

In a Nutshell

_matmul, _apply, _transpose etc. can now be implemented by overriding the corresponding methods in a subclass instead of passing the implementations in the constructor. The previous approach is still possible via the new LambdaLinearOperator.

Detailed Description

  • Refactor LinearOperator such that method implementations are provided via subclass overrides
  • Introduce LambdaLinearOperator subclass which provides support for the old approach of passing method implementations via the constructor
  • Replace all previous uses of LinearOperator with LambdaLinearOperator to maintain compatibility

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@marvinpfoertner marvinpfoertner left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! 🙏🏻

Some general points:

  • Instead of just saying "You may implement this method in a subclass.", we should probably state that overriding the default implementation only makes sense if we know a more efficient implementation (in terms of memory or computation time) than the fallback.
  • Can you get rid of the double underscores in the cache variables? We don't really need those anymore

src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
src/probnum/linops/_linear_operator.py Outdated Show resolved Hide resolved
@JonathanWenger JonathanWenger added the linops Issues related to linear operators label Nov 7, 2022
@marvinpfoertner marvinpfoertner added this to In progress in ProbNum Development via automation Nov 7, 2022
@timweiland timweiland requested review from marvinpfoertner and removed request for JonathanWenger November 7, 2022 23:10
@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #735 (ae8b0dc) into main (3999850) will increase coverage by 0.13%.
The diff coverage is 98.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #735      +/-   ##
==========================================
+ Coverage   90.27%   90.40%   +0.13%     
==========================================
  Files         197      197              
  Lines        7445     7454       +9     
  Branches      989      966      -23     
==========================================
+ Hits         6721     6739      +18     
+ Misses        487      481       -6     
+ Partials      237      234       -3     
Impacted Files Coverage Δ
src/probnum/linops/_utils.py 69.23% <0.00%> (ø)
src/probnum/linops/_linear_operator.py 88.05% <98.56%> (+2.13%) ⬆️
src/probnum/linalg/solvers/matrixbased.py 73.75% <100.00%> (ø)
src/probnum/linops/__init__.py 100.00% <100.00%> (ø)
src/probnum/linops/_arithmetic_fallbacks.py 85.57% <100.00%> (ø)
src/probnum/linops/_kronecker.py 89.80% <100.00%> (-0.91%) ⬇️
src/probnum/linops/_scaling.py 82.73% <100.00%> (+0.17%) ⬆️

@JonathanWenger
Copy link
Contributor

JonathanWenger commented Nov 8, 2022

Great job on your first PR @timweiland. To fix the formatting and isort issues, once you've installed tox you can run

tox -e format

To automatically keep code formatted you can also install the formatting libraries directly via

cd probnum
pip install -r formatting-requirements

You can then run black and isort on save in VSCode for example. To avoid comitting files which are incorrectly formatted in the future you can also alternatively install the provided precommit hooks to check for correct formatting prior to comitting (see https://probnum.readthedocs.io/en/latest/development/pull_request.html#pre-commit-hooks)

@timweiland
Copy link
Collaborator Author

Thanks @JonathanWenger! I've fixed all of the problems now except for one which I'm unsure about:
Pylint complains that in this code, self.T is not callable (E1102): y = (self.T)(x, axis=-1)
But clearly, self.T returns a LinearOperator which is callable: def T(self) -> "LinearOperator":
Do you know how to fix this? It seems that it might have something to do with the @property decorator.

@timweiland
Copy link
Collaborator Author

Thanks @JonathanWenger! I've fixed all of the problems now except for one which I'm unsure about: Pylint complains that in this code, self.T is not callable (E1102): y = (self.T)(x, axis=-1) But clearly, self.T returns a LinearOperator which is callable: def T(self) -> "LinearOperator": Do you know how to fix this? It seems that it might have something to do with the @property decorator.

I talked to @marvinpfoertner about this and we agreed to simply make pylint ignore this line.

ProbNum Development automation moved this from In progress to Review in progress Nov 8, 2022
Copy link
Collaborator

@marvinpfoertner marvinpfoertner left a comment

Choose a reason for hiding this comment

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

Thanks again for doing this! :) Congrats on you first ProbNum PR! 🎉

timweiland and others added 9 commits November 8, 2022 18:41
_matmul, _apply, _transpose etc. can now be implemented by overriding
the corresonding methods in a subclass instead of passing the
implementations in the constructor. The previous approach is still
possible via the new LambdaLinearOperator.
Co-authored-by: Marvin Pförtner <marvin.pfoertner@icloud.com>
@marvinpfoertner marvinpfoertner merged commit ce4bbba into probabilistic-numerics:main Nov 8, 2022
ProbNum Development automation moved this from Review in progress to Done Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linops Issues related to linear operators
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants