Skip to content

Conversation

@lkdvos
Copy link
Member

@lkdvos lkdvos commented Oct 12, 2025

This PR aims to address #66:

The starting point is to have two main modes of operation, similar to what we had before but with some small changes:

  1. Being able to easily vary the kind of factorization, in which case you provide a set of keywords for either of the possible kinds that can be chosen, and the correct ones are taken into account while the other ones are being ignored
  2. Being able to specify an algorithm, where the kind of factorization is determined automatically.

Going from there, I made a number of changes to help achieve this goal.

  • I promoted left_orth and friends to full @functiondef implementations. This means that they all follow the same procedure as the other functions, and importantly this cleanly separates out the algorithm selection procedure from the implementation.
  • This ensures that the decomposition type is selected before the output is initialized, leaving the option to specialize what kinds of output are generated
  • I added/kept the helper functions left_orth_qr etc, with some boilerplate to provide good default implementations, but leaving the option to specialize here.

To achieve this while keeping type stability intact as much as possible, I had to slightly rework the interface. In particular, as the @functiondef algorithm selection procedure depends/specializes mostly on the alg keyword, I had a hard time making things type stable by only using the other keyword arguments. Therefore, I adopted alg as the driving selection mechanism:

  • alg::Symbol replaces the previous kind::Symbol, and in this case the selection procedure is based on the alg_qr, alg_svd etc keyword arguments.
  • alg::AbstractAlgorithm uses a "trait-like" system to determine the factorization kind
  • the default alg::Nothing=nothing selects either a QR or SVD based decomposition based on the presence of trunc.

This necessarily removes the ability to select an algorithm type through a symbol, as is possible for example for the qr_compact(A; alg=:LAPACK_HouseholderQR). I don't think this is something I can make work in a type-stable manner (I definitely tried a bunch of things, but short of inlining everything and using generated functions, I don't see how I can convince the compiler to constant-propagate that symbol, which would then still not really be type-stable if that symbol is meant to be varied)

Some other relevant changes:

  • I altered the TruncationIntersection implementation to use a recursive approach, which seems to improve type-stability
  • I reworked some of the tests, by separating out the LinearMap implementation to a separate file, and removing some of the checks on the outputs of left_orth etc aliasing the inputs. As our interface explicitly specifies this is not behavior that should be depended on, I feel like it is warranted that our tests should not test for this?
  • The default left_null_svd implementation now ignores the specified output, as this cannot reasonably be used since the size varies. This doesn't preclude us from adding this back in for specialized cases, and the infrastructure is still there, albeit bypassed through N = nothing.

I think overall I've removed some boilerplate code, and the added boilerplate should be beneficial for downstream packages, and low-maintenance on our side here.
I'm obviously curious about any comments and suggestions for possible improvements.
Some remarks that I've encountered which might need addressing:

  • The truncation strategies for the null spaces are a bit funny, in the sense that we might want to consider them as being the inverted case of what they are for the truncated decompositions (by explicitly returning the complement of the ind computed currently). One thing we may therefore consider is adding a findtruncated_null on top of the findtruncated and findtruncated_svd, which has precisely this interpretation. I'm not sure if this would overall make a big change, but it might make it easier to reason about.
  • There are some gymnastics required to make the two use-cases fit together in a single function, and I can't really see a use-case for wanting both of these uses at the same time. Do we want to just split this into two separate functions? left_orth1(A; kind, trunc, alg_svd, ...) and left_orth2(A; alg, trunc, kwargs...) just requires us to come up with 2 names, but is otherwise a bit more straightforward.
  • Do we want to rework the alg::Nothing mode to select an alg::Symbol to match the presence of trunc, and then dispatch to the alg::Symbol mode? Currently these are slightly distinct, as alg::Nothing uses all kwargs for algorithm selection, while alg::Symbol uses the alg_qr etc.

@lkdvos lkdvos linked an issue Oct 12, 2025 that may be closed by this pull request
@lkdvos lkdvos requested review from Jutho and mtfishman October 12, 2025 14:42
@lkdvos lkdvos self-assigned this Oct 12, 2025
@Jutho
Copy link
Member

Jutho commented Oct 12, 2025

I haven't reviewed anything, but could you change the JET compat entry to "0.9,0.10" and hope that this fixes the Julia 1(.12) tests?

@lkdvos lkdvos added documentation Improvements or additions to documentation enhancement New feature or request breaking Constitutes a breaking change labels Oct 12, 2025
@github-actions
Copy link

After the build completes, the updated documentation will be available here

@codecov
Copy link

codecov bot commented Oct 12, 2025

Codecov Report

❌ Patch coverage is 79.76879% with 35 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/implementations/orthnull.jl 80.30% 13 Missing ⚠️
src/interface/orthnull.jl 76.00% 12 Missing ⚠️
src/algorithms.jl 77.27% 5 Missing ⚠️
src/interface/decompositions.jl 60.00% 4 Missing ⚠️
src/interface/truncation.jl 85.71% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/implementations/svd.jl 92.36% <ø> (ø)
src/implementations/truncation.jl 91.30% <100.00%> (-2.12%) ⬇️
src/interface/eig.jl 85.71% <ø> (ø)
src/interface/eigh.jl 80.00% <ø> (ø)
src/interface/svd.jl 80.00% <ø> (ø)
src/interface/truncation.jl 70.00% <85.71%> (+1.81%) ⬆️
src/interface/decompositions.jl 63.63% <60.00%> (-36.37%) ⬇️
src/algorithms.jl 90.47% <77.27%> (-0.15%) ⬇️
src/interface/orthnull.jl 76.00% <76.00%> (-24.00%) ⬇️
src/implementations/orthnull.jl 81.42% <80.30%> (-9.49%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Jutho
Copy link
Member

Jutho commented Oct 15, 2025

I think this all looks very reasonable and probably cleaner and shorter. I'll take a closer look to interface/orthnull.jl tonight but I don't expect further comments.

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

Labels

breaking Constitutes a breaking change documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

left_orth and right_orth interface improvements

3 participants