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

Disambiguate mul! for matvec and matmat through indirection #52837

Merged
merged 6 commits into from
Jan 10, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jan 10, 2024

This forwards mul! for matrix-matrix and matrix-vector multiplications to the internal _mul!, which is then specialized for the various array types. With this, packages would not encounter any method ambiguity when they define

mul!(::AbstractMatrix, ::MyMatrix, ::AbstractMatrix, alpha::Number, beta::Number)

This should reduce the number of methods that packages need to define to work around ambiguities with LinearAlgebra.

There was already an existing internal function named _mul!, but the new methods don't clash with the existing ones, and since both sets of methods usually forward structured multiplications to specialized functions, it's fitting for them to have the same name.

@jishnub jishnub added domain:linear algebra Linear algebra domain:arrays [a, r, r, a, y, s] needs tests Unit tests are required for this change labels Jan 10, 2024
@jishnub jishnub removed the needs tests Unit tests are required for this change label Jan 10, 2024
@dkarrasch
Copy link
Member

IIUC, then this moves the "internal LinearAlgebra" disambiguation to _mul!. Is that correct? So packages don't need to disambiguate with the matrix subgroups BandedMatrix and *Triangular? I think this is worth a NEWS entry, to explain which type of ambiguity used to exist, and is now removed. I suspect one of the main beneficiaries will be ArrayLayouts.jl.

@jishnub
Copy link
Contributor Author

jishnub commented Jan 10, 2024

Yes, you're right. I suspect FillArrays and other similar array packages that support linear algebra would also benefit from this, but none more than ArrayLayouts.

@dkarrasch dkarrasch added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 10, 2024
@jishnub jishnub merged commit 2afc20c into master Jan 10, 2024
8 checks passed
@jishnub jishnub deleted the jishnub/muldisambiguate branch January 10, 2024 16:38
@jishnub jishnub removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants