JuliaLang
Fix errant lmul! for tridiagonal and triangular
This method is incorrect. Firstly, the current implementation doesn't act in-place. Secondly, the result can't be stored in-place into a triangular matrix in general. This PR changes the implementation to convert the tridiagonal matrix to a Diagonal or a Bidiagonal one before attempting the lmul!.
Currently,
julia> T = Tridiagonal([1,2], [1,2,3], [1,2]); U = UpperTriangular(fill(2, 3, 3));
julia> lmul!(T, U)
3×3 Matrix{Int64}:
2 4 4
2 6 10
0 4 10
julia> U # not updated
3×3 UpperTriangular{Int64, Matrix{Int64}}:
2 2 2
⋅ 2 2
⋅ ⋅ 2
julia> parent(U) # except for the underlying storage
3×3 Matrix{Int64}:
2 2 2
0 2 2
0 0 2
After this,
julia> lmul!(T, U)
ERROR: ArgumentError: matrix cannot be represented as Bidiagonal
I'm unsure if we want this method at all, since there isn't a corresponding rmul!, but I've left it there to avoid breakages, if any.
I stumbled upon this method many times, and have always wondered what it's good for. I believe I have tried to remove it, but then something failed. And so I tried every time I forgot about my previous attempt. :rofl: Since this is very old, I think we need to run pkgeval, to see if there are expectations silently built into the ecosystem. Perhaps the ! used to indicate that the underlying storage is potentially altered, not that the multiplication is in-place?
Should we run a pkgeval by removing the method? I fear that retaining this method might introduce hard-to-find correctness bugs, and it's better for the method to fail. We should also probably be more explicit in the docstrings that the input argument B will be overwritten with the result A * B. This is the point of lmul! anyway, as a method like lmul!(A, B) = A * B serves little purpose.
Should we run a pkgeval by removing the method?
Yes, let's try that. Maybe I wasn't brave enough to just remove tests that exist for the sake of coverage. One big issue, as you say, is that you need to do result = lmul!(Td, Tr) to have a handle for the result, which is in stark contrast to how people would normally use lmul!.
The package evaluation job you requested has completed - possible new issues were detected. The full report is available.
@nanosoldier runtests(["EulerAngles", "Groups", "Pyehtim", "DelayEmbeddings", "OptimalPortfolios", "Miter", "ThermodynamicIntegration", "Polynomials4ML", "IRKGaussLegendre", "OrdinaryDiffEqHighOrderRK", "OrdinaryDiffEqQPRK", "OrdinaryDiffEqFeagin", "OrdinaryDiffEqLowOrderRK", "OrdinaryDiffEqStabilizedRK", "OrdinaryDiffEqNordsieck", "OrdinaryDiffEqAdamsBashforthMoulton", "OrdinaryDiffEqSSPRK", "MCMCTempering", "OceanRobots", "MicroCanonicalHMC", "FMIImport", "BiochemicalAlgorithms", "OptimizationOptimJL", "Simpsons", "JumpProblemLibrary", "RealTimeAudioDiffEq", "GeneralizedSasakiNakamura", "VLBIPlots", "RobustBlindVerification", "ConceptualClimateModels", "BloqadeSchema"])
The package evaluation job you requested has completed - possible new issues were detected. The full report is available.
Seems like we can go ahead and remove it. I just wonder if this should really be backported.
Probably best to not backport this. My original idea was to restrict it to cases where it does work, instead of removing it altogether