Skip to content

Conversation

@dkarrasch
Copy link
Member

This PR brings back collect for AbstractQs, and redirects it to efficient matrix generation code. It also removes two further mul! methods for AdjointQ for which I don't see why they can't simply fall back to the generic AbstractQ methods, but let's see what pkgeval says.

Closes JuliaLang/LinearAlgebra.jl#1006.

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

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch
Copy link
Member Author

dkarrasch commented May 22, 2023

@Seelengrab I thought about the request to provide the AbstractMatrix interface without subtyping a bit more. I'm not sure how practical that is. There are about 300 methods with AbstractMatrix. Do we really mean to provide all of those methods instead of the former fallback? Like, for instance, the trace function:

julia> using BenchmarkTools

julia> @btime tr($Q);
  5.027 ms (5 allocations: 3.82 MiB)

julia> @btime tr($M);
  528.021 ns (0 allocations: 0 bytes)

where Q is a qr.Q and M is its matrix representation?

The required functions for an AbstractArray subtype, though, include only size and getindex, everything else seems to be optional. I have come to appreciate the collect function though as a functional way of creating the square matrix representation, in contrast to Matrix(Q) which may yield a rectangular version. So providing collect makes much sense, and I'll mention it in the docstring. As for the "rest" of the AbstractArray interface, I'd like to get a feeling of how much of it we want to keep.

@Seelengrab
Copy link
Contributor

To be clear, I agree with the reasoning that Q is not a matrix (and shouldn't be, but not due to the time complexity). The question whether or not a Q or R is a matrix isn't the main issue, but instead purely that this was an observable breaking change, to the extent that a user asked about why it broke something in their (expected to be supported) workflow. That the workflow is "odd" is not for us to decide though - if users couldn't rely on subtyping relationships to stay stable between minor versions, how are they supposed to write code with dispatch that they could expect to continue working until we eventually do branch 2.0?

My personal reasoning is this - do we think that a time complexity larger than O(1) for some operations are grounds for not being <: AbstractArray? After all, that would mean something like DistributedArray, with a time complexity of O(whatever-your-network-feels-like) also shouldn't subtype <: AbstractArray, and similar for other more complex arrays.

So this is a bit of a weird situation, because it's (probably/likely still) a good change (a Factor of a matrix really shouldn't be a matrix itself, it should only be combined with the other factor or explicitly converted to a matrix), but this can't IMO really be done in a non-breaking release, locked to the julia version. Maybe the situation would be different if LinearAlgebra wasn't a bundled stdlib and could make a breaking release independent of the julia version, but that's not the situation we're in right now 🤷

@dkarrasch dkarrasch force-pushed the dk/abstractq_cleanup branch from ea00ef7 to cd1528b Compare May 23, 2023 07:56
@dkarrasch
Copy link
Member Author

Ok, I understand that stability concern well. I'm just trying to figure out how far we want to go "back". The experience in adjusting the ecosystem was the following. I could have avoided almost any breakage by defining (i) collect and (ii) exact and approximate comparison to other AbstractQ and AbstractMatrix objects. Item (i) is already fixed in this PR as is. I'll include (ii) as well. Would that be sufficient to go with the AbstractQ overhaul?

@Seelengrab
Copy link
Contributor

My gut says "this should have been a deprecation and only subsequent removal in a breaking (excised stdlib?) release", but that has the issue that it prevents you from using the qr name to just give you the new object. It really is a rock and a hard place..

If implementing (ii) makes your tr and similar examples work (albeit slowly), I think that would be sufficient, yes. Perhaps with an additional deprecation warning, recommending tr(Matrix(Q)) instead, to make that conversion explicit? Is that appropriate? My concern is mostly that collect(Q) is just a symptomatic case that someone ran into fairly early, with other cases popping up after the fact..

@dkarrasch
Copy link
Member Author

My concern is mostly that collect(Q) is just a symptomatic case that someone ran into fairly early

I really do understand that concern, but I don't share it. I think most if not all users have used AbstractQ objects in a rather different way than how they used matrices.

If implementing (ii) makes your tr and similar examples work (albeit slowly)

We would then need to provide methods for AbstractQ that used to work earlier only due to the AbstractMatrix subtyping and fallbacks. Technically, we are then talking about up to 700 methods, see the output of

using LinearAlgebra
methodswith(LinearAlgebra.AbstractQ, supertypes=true)

I'm exagerating, of course, it's less than that, but still. There is no simple way of making previous AbstractMatrix fallbacks work for AbstractQ but to add for each and every function another method that accepts AbstractQ and converts it to a matrix first. The "converse" way would be to deprecate up to 700 methods that, behind the scenes, have always converted the Q into a matrix elementwise, albeit the most expensive way you can think of.

@dkarrasch
Copy link
Member Author

To summarize: this PR adds those methods whose "lack" in the first major PR triggered a sweep through the ecosystem to "fix" the lack of functionality: (i) comparison with matrices and (ii) collect. If these had been included in the first PR, we wouldn't have seen any package breakage, except for very few instances of iteration, at least one of which was even mistaken. So, with these additions here, I am convinced that the technically breaking AbstractQ changes are practically not breaking. I'd like to suggest to merge this PR and enjoy the disentangled AbstractQ-AbstractMatrix.

@dkarrasch dkarrasch added this to the 1.10 milestone Jun 23, 2023
@dkarrasch dkarrasch changed the title Bring back collect(::AbstractQ), rm mul! methods Mild AbstractQ review and refactoring Jun 23, 2023
@dkarrasch
Copy link
Member Author

Ha, I managed to reduce the "extend by zeros flexibly"-code significantly. This now established the following call chain:

* with at least one AbstractQ factor
->
3-arg mul! which copies the non-Q-factor to the output and overwrites the other elements with zero
->
lmul! or rmul! as appropriate and as required by the API.

Now, some AbstractQ types are flexible in size under * (sometimes from the left, the LQPackedQ from the right). This flexibility can now be encoded by providing a qsize_check function for own AbstractQ subtypes. By default, AbstractQs are treated as matrices and size compatibility is checked based on whatever size returns. In summary, packages may hook into the "flexible size mechanism" by overloading qsize_check, and otherwise rely on the generic methods, just like the LinearAlgebra subtypes of AbstractQ!

@dkarrasch
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch
Copy link
Member Author

@nanosoldier runtests(["DiffOpt", "MLFlowLogger", "JumpProcesses", "QuantumAnnealingAnalytics", "LuaCall", "Decapodes", "FiniteVolumeMethod1D", "PostNewtonian", "Pyehtim", "AlgebraicRL", "VoronoiFVMDiffEq", "RegularizedProblems"])

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch dkarrasch merged commit dd1f03d into master Jun 25, 2023
@dkarrasch dkarrasch deleted the dk/abstractq_cleanup branch June 25, 2023 12:01
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.

Cannot collect Q factor of QR factorization

4 participants