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

Edits to generic add, BlockDiagLinearOperator's matmul, and documentation #10

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

SebastianAment
Copy link
Collaborator

This PR makes three small changes:

  1. It introduces a size check in BlockDiagLinearOperator's matmul. This takes care of an edge case where the blocks of two block diagonal operators have different sizes, even though the operators have the same size. E.g. One operator has twice as many blocks of half the size as the other.

  2. It introduces a deepcopy in the generic __add__ method for addition with a ZeroLinearOperator, see the discussion here.

  3. It makes edits to the docs, which referenced the old lazy submodule, instead of the new operators.

@SebastianAment
Copy link
Collaborator Author

While thinking about the sizes in matmul, I noticed that the docs of BlockLinearOperator imply that the blocks should be square. If we don't want to support rectangular blocks, I'd also add a quick check in the constructor of BlockLinearOperator to make sure the blocks are indeed square.

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.

lgtm, thanks for fixing the docs. Let's add that check for blocks being square as well (that has so far been an implicit assumption for most of this library anyway, though ideally we can relax this going forward).

@Balandat Balandat merged commit 997156d into cornellius-gp:main Sep 8, 2022
@SebastianAment SebastianAment deleted the add-matmul-edits branch September 8, 2022 19:16
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.

2 participants