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

Add short path for LinearOperator.repeat() #70

Merged
merged 4 commits into from
Jul 15, 2023

Conversation

Turakar
Copy link
Contributor

@Turakar Turakar commented Jun 8, 2023

This adds a short path for LinearOperator.repeat() if we would otherwise create a new BatchRepeatLinearOperator which would be a no-op.

repeat() is called like this for example by the constructor of KroneckerProductLinearOperator.

By reducing one level of indirection, we gain two benefits: a) slightly better performance (obviously), and b) some more optimizations by calling native functions instead of default LinearOperator functions, e.g. for _diagonal().

Copy link
Collaborator

@Balandat Balandat left a comment

Choose a reason for hiding this comment

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

This makes sense to me. Should we add a unit test for this to check that we indeed return the same object rather than a BatchRepeatLinearOperator in this case?

Copy link
Member

@gpleiss gpleiss left a comment

Choose a reason for hiding this comment

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

@Turakar please add a unit test like @Balandat suggests, and then we'll merge!

@Turakar
Copy link
Contributor Author

Turakar commented Jul 7, 2023

Seems like there were no unit tests before, so I added tests for both cases: A No-Op case and a true case. I also allowed the repeat(1, 1) case as this is a no-op, too.

@Turakar Turakar requested a review from gpleiss July 14, 2023 12:14
@Balandat Balandat merged commit e915cc0 into cornellius-gp:main Jul 15, 2023
5 checks passed
@Turakar Turakar deleted the smallopt_repeat branch July 16, 2023 11:38
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