More matrix factorizations

[mtfishman]

This moves logic for selecting the proper MatrixAlgebraKit.jl matrix factorization from the tensor level to the matrix level by introducing a submodule TensorAlgebra.MatrixAlgebra. This enables making some of the tensor factorization definitions more compact by generating them in an @eval loop. It also introduces new shorthand factorizations polar and orth, where left or right can be selected with a keyword argument.

[codecov]

Codecov Report

Attention: Patch coverage is 96.55172% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.42%. Comparing base (43553e3) to head (89c8d33).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/MatrixAlgebra.jl	94.87%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
- Coverage   95.91%   95.42%   -0.49%     
==========================================
  Files          14       15       +1     
  Lines         489      459      -30     
==========================================
- Hits          469      438      -31     
- Misses         20       21       +1     

Flag	Coverage Δ
docs	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lkdvos]

Definitely a good idea to have a submodule for this.

I feels a bit odd that there are some functions that exist there and some that do not (eg svdvals), but I guess MatrixAlgebra is not meant to be used outside of this package and then this does not matter?

[mtfishman]

@lkdvos thanks for the thoughtful comments, I'll go through them.

[mtfishman]

I feels a bit odd that there are some functions that exist there and some that do not (eg svdvals), but I guess MatrixAlgebra is not meant to be used outside of this package and then this does not matter?

Good catch. I originally left out svdvals since it would just be the same definition as MatrixAlgebraKit.svd_vals, and I was thinking of TensorAlgebra.MatrixAlgebra as new functionality added on top of MatrixAlgebraKit. However, for completeness I added it in. And yes, TensorAlgebra.MatrixAlgebra right now is meant to mostly be used internally, though I added exports for all of the TensorAlgebra.MatrixAlgebra functions (from TensorAlgebra.MatrixAlgebra, not from TensorAlgebra) which allows us to do using TensorAlgebra.MatrixAlgebra and access those functions. I also added tests for them.

[mtfishman]

@lkdvos I think I've addressed all of your comments, let me know what you think.

[lkdvos]

Overall looks good to me!

Just wanted to share some thoughts I had while reading through this, in the end this is completely subjective and shouldn't have any major implications, but I did want to know how you feel about this:

While the @eval loops are nice, there are definitely also some drawbacks associated to them:

• I do feel it becomes more involved to read the source code and immediately capture what is going on.
• Static analysis tools have a harder time dealing with this, for example Cthulhu combined with VSCode has a harder time mapping type analysis to source code statements (since the source code statements are somewhat hidden).
• It becomes somewhat tricky to debug when you want to insert some print statements.

All of this just to highlight that macros don't come without drawbacks.
Here, in most cases I do think they are warranted and nice, but for example for the "loop"s over eigen and eigen!, and similarly the other in-place vs non-inplace versions I'm a bit more on the fence: the benefit in terms of convenience might not outweigh the drawbacks. Ultimately I don't think there is a "right" choice, but I don't think it's bad to bring this up and actively consider it every now and again :)

[mtfishman]

That's fair, I definitely don't like to overuse macros and it is good to assess where it is really worth it. Here I liked it because it guarantees that the logic across definitions are exactly the same. Some of definitions I think are long enough to warrant using @eval since I don't want to repeat the same logic multiple times, while as you say others are shorter and maybe don't warrant it, but for consistency I just defined all of them through @eval so I didn't have to make a judgement call about where that line is. I also am anticipating we may add more features and options to some of the definitions (particularly factorize), and I think having them defined with @eval will help to make that easier (as you saw, it is pretty easy to forget to forward the right set of keyword arguments and also it is tough testing every combination of options). Anyway, I'll leave it as it is for now, but I'm happy to reassess that choice going forward.

[mtfishman]

@ogauthe this will require updating #50 since it changes some factorization definitions, but I think that won't be very complicated (if anything it should be easier now since I merged certain definitions together).