-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor orthogonalization and nullspace interface #77
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
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
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? |
|
After the build completes, the updated documentation will be available here |
Jutho
reviewed
Oct 14, 2025
Jutho
reviewed
Oct 14, 2025
Jutho
reviewed
Oct 15, 2025
Jutho
reviewed
Oct 15, 2025
Jutho
reviewed
Oct 15, 2025
Member
|
I think this all looks very reasonable and probably cleaner and shorter. I'll take a closer look to |
This was referenced Oct 15, 2025
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
kindof 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 ignoredkindof factorization is determined automatically.Going from there, I made a number of changes to help achieve this goal.
left_orthand friends to full@functiondefimplementations. 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.left_orth_qretc, 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
@functiondefalgorithm selection procedure depends/specializes mostly on thealgkeyword, I had a hard time making things type stable by only using the other keyword arguments. Therefore, I adoptedalgas the driving selection mechanism:alg::Symbolreplaces the previouskind::Symbol, and in this case the selection procedure is based on thealg_qr,alg_svdetc keyword arguments.alg::AbstractAlgorithmuses a "trait-like" system to determine the factorization kindalg::Nothing=nothingselects either a QR or SVD based decomposition based on the presence oftrunc.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:
TruncationIntersectionimplementation to use a recursive approach, which seems to improve type-stabilityLinearMapimplementation to a separate file, and removing some of the checks on the outputs ofleft_orthetc 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?left_null_svdimplementation 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 throughN = 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:
indcomputed currently). One thing we may therefore consider is adding afindtruncated_nullon top of thefindtruncatedandfindtruncated_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.left_orth1(A; kind, trunc, alg_svd, ...)andleft_orth2(A; alg, trunc, kwargs...)just requires us to come up with 2 names, but is otherwise a bit more straightforward.alg::Nothingmode to select analg::Symbolto match the presence oftrunc, and then dispatch to thealg::Symbolmode? Currently these are slightly distinct, asalg::Nothinguses allkwargsfor algorithm selection, whilealg::Symboluses thealg_qretc.