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

ENH: added overlap parameter in scipy.linalg.block_diag. #17884

Closed
wants to merge 3 commits into from

Conversation

vetschn
Copy link
Contributor

@vetschn vetschn commented Jan 30, 2023

What does this implement/fix?

This PR adds an overlap parameter to scipy.linalg.block_diag. This allows one to specify a number of rows/cols that the matrices should overlap additively. I found this piece of code very useful when constructing finite-element system-matrices from the matrices of coupled periodic sub-domains.

Additional information

I realize the use-cases for this parameter may be very limited and specific to my problem, but I could not find a solution in any other package out there and think others could find it useful as well.

It may even be interesting to add an option to specify the type of operation that should be performed on overlapping elements but that may be overkill.

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @vetschn. Interesting ideal. Before reviewing, we would need to go through the mailing list to validate the proposal (it's a new feature with an API change, so needs to be agreed upon before). Could you send an email?

@tupui tupui added enhancement A new feature or improvement scipy.linalg needs-decision Items that need further discussion before they are merged or closed labels Jan 30, 2023
@rgommers
Copy link
Member

Two comments on the proposed overlap = <int> semantics:

  • It looks like this only applies to the case of 2 input arrays, with multiple arrays it probably doesn't do what you want.
  • A diagonal shift is a subset of horizontal and vertical shifts, and I don't see why diagonal shifts are the only case of interest.

It's also relatively straightforward to do this without the new overlap parameter, right? You can write a function that's only a few lines long, creates two matrices separately of the right shape, and then adds them. Something like:

def block_diag_overlap(A, B, overlap):
    A_tmp = block_diag(A, np.zeros((B.shape[0] - overlap, B.shape[1] - overlap), dtype=B.dtype))
    B_tmp = # the reverse
    return A_tmp + B_tmp

So I'm leaning towards this not being worth adding at the moment.

@stefanv
Copy link
Member

stefanv commented Feb 3, 2023

So I'm leaning towards this not being worth adding at the moment.

This would work especially well with sparse matrices. The PR is a cute idea, but it adds quite a bit of mental overhead to maintaining what would otherwise be a straightforward implementation.

@tupui
Copy link
Member

tupui commented Feb 3, 2023

Alright then it seems like we should close the PR as it is. Thanks again @vetschn. Without making a full/complex implementation, one might consider a doc improvement instead-with what Ralf wrote.

@tupui tupui closed this Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement needs-decision Items that need further discussion before they are merged or closed scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants