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

muladd(matrix, matrix, scalar) gives unexpected result #1162

Open
stevengj opened this issue Jan 1, 2025 · 3 comments
Open

muladd(matrix, matrix, scalar) gives unexpected result #1162

stevengj opened this issue Jan 1, 2025 · 3 comments
Labels
breaking This change will break code maths Mathematical functions

Comments

@stevengj
Copy link
Member

stevengj commented Jan 1, 2025

This seems wrong:

julia> muladd(zeros(2,2), zeros(2,2), 3)
2×2 Matrix{Float64}:
 3.0  3.0
 3.0  3.0

It's not clear that muladd(X, Y, z) should even be defined when z is a scalar, since it is defined as X * Y + z and we stopped doing implicit vector + scalar broadcasting a long time ago.

This was added in JuliaLang/julia#37065 by @mcabbot, but I don't see much discussion of the scalar case in that PR?

@stevengj stevengj added the maths Mathematical functions label Jan 1, 2025
@mcabbott
Copy link
Collaborator

mcabbott commented Jan 2, 2025

That PR changed the definition to X * Y .+ z, with the restriction that the answer not be larger than X * Y alone. The motivating case (as in the top post) wanted z::Vector. (That PR was a bit of a mess, sorry, as it was written under the belief that there no existing non-Number uses... was tightened in follow-up PRs.)

@LilithHafner
Copy link
Member

For the evalpoly algorithm we'd want this to return [3 0; 0 3]. Subjectively, I think that [3 0; 0 3] makes more sense mathematically and [3 3; 3 3] is more useful for hand coded neural networks and other ML stuff.

Unfortunately, I don't think this is fixable in a non-breaking way. We could try nanosoldier, but I'm not optimistic.

@stevengj
Copy link
Member Author

stevengj commented Jan 3, 2025

My preference would be for muladd(matrix, matrix, scalar) (or muladd(matrix, matrix, vector)) to simply throw an error, since we don't usually do implicit broadcasting in arithmetic operations.

But I agree that it's maybe too late to change in Julia 1.x.

@stevengj stevengj added the breaking This change will break code label Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code maths Mathematical functions
Projects
None yet
Development

No branches or pull requests

3 participants