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

Reduce mul! methods related to "banded" matrices #49666

Merged
merged 1 commit into from
May 6, 2023
Merged

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented May 6, 2023

I was encouraged by @blegat to further reduce the number of mul! methods, so here we go. I think we can do this by defining larger "groups" of types, catch any multiplication where they are involved and redirect to its own multiplication function(!), so here it's _mul!. This may be of interest to @dlfivefifty: I grouped together all the "banded" structured matrices (remember we discussed subtyping (some of) them to AbstractTriangular), and called the group BandedMatrix. Currently, this is a simple union, but we may want to consider actually introducing an abstract type and make these subtype it. And we may want to call the "banded matrix-related" _mul! bandedmul!. I can guess you can see where this is going. Does that sound like a good direction or should we avoid any interference with *BandedMatrices.jl?

In the big scheme of things, we can then resolve ambiguities with other "groups"(like wrapped (typically dense) matrices à la triangular, upper Hessenberg) at the larger scale, and ambiguity resolution within the groups happens at the _mul!/bandedmul! scale, so doesn't increase the number of mul! methods.

This PR reduces the number of mul! methods from 88 to 67, but it excludes the triangular part, which is a huge source of methods. I''ll take care of that part in my triangular overhaul #43972.

@dkarrasch dkarrasch added the linear algebra Linear algebra label May 6, 2023
@dkarrasch
Copy link
Member Author

Let's merge this and discuss naming over at #43972.

@dkarrasch dkarrasch merged commit b69a417 into master May 6, 2023
@dkarrasch dkarrasch deleted the dk/bandedmul branch May 6, 2023 14:08
@dlfivefifty
Copy link
Contributor

dlfivefifty commented May 6, 2023

Why not just make them subtypes of AbstractBandedMatrix which then BandedMatrices.jl can use?

(though that’d take work to implement since then we’d lose the LayoutMatrix functionality)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants