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

dispatch.jl: remove LinearAlgebra.mul! overloads #182

Closed
wants to merge 2 commits into from
Closed

Conversation

odow
Copy link
Member

@odow odow commented Nov 21, 2022

This one is a little more controversial. Originally added by @blegat in #16

At first glance, the problem with this is that we're missing a bunch of methods. Triangular, Symmetric, etc.

I started writing a replacement that would loop over the types in LinearAlgebra. But this just ended up creating a lot of ambiguities because SparseArrays does something similar:
https://github.com/JuliaSparse/SparseArrays.jl/blob/311b4b4130d9f28a6b5eb55cb7c818e4f7858719/src/linalg.jl#L30-L56

So then I stepped back and asked, why are we even implementing these methods? MutableArithmetics has plenty of support for operate_to!(C, *, A, B), and trying to intercept all of these weird array types based on the element type is ripe for problems.

I couldn't really find examples of packages where we call this (it's quite hard to search for mul!), so this might lead to a performance regression in some user-code, but arguably they should be using the proper MA API instead of trusting this weird fallback in LinearAlgebra?

@odow
Copy link
Member Author

odow commented Nov 21, 2022

These are also candidates to remove: #181

@blegat
Copy link
Member

blegat commented Nov 21, 2022

When the user calls A * B outside the macro, it ends up calling LinearAlgebra.mul! which either fails because it incorrectly preallocate the resulting vector for JuMP types (see #47) or is slow because it does not exploit mutability so JuMP needs this

@odow
Copy link
Member Author

odow commented Nov 21, 2022

When the user calls A * B outside the macro

Yes. I don't see why we should intercept calls outside the macros.

I guess if this is a breaking change, we can not merge it, but for MutableArithmetics 2.0, I'm not in favor.

@odow
Copy link
Member Author

odow commented Nov 21, 2022

Our other problem is that I can remove all of these methods and the tests still pass.

@odow
Copy link
Member Author

odow commented Nov 21, 2022

I guess we needed this because the old rewrite macro didn't rewrite every expression. It would often leave complicated expressions as-is, so x' * (A * x) might leave A * x alone, and that would need the fallback.

@odow
Copy link
Member Author

odow commented Nov 21, 2022

I'm adding some tests to JuMP that demonstrates why we need these: jump-dev/JuMP.jl#3132

@odow
Copy link
Member Author

odow commented Nov 21, 2022

Okay. The new JuMP tests are now failing with the removal of these methods.

@odow odow closed this Nov 21, 2022
@odow odow deleted the od/dispatch-4 branch November 21, 2022 23:08
@blegat
Copy link
Member

blegat commented Nov 22, 2022

It is suprising that the JuMP tests were not failing indeed, it's good to have jump-dev/JuMP.jl#3132

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

Successfully merging this pull request may close these issues.

2 participants