Skip to content
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

Add LinearAlgebra: lu, lq, svd, qr #24

Merged
merged 14 commits into from
Feb 11, 2021
Merged

Add LinearAlgebra: lu, lq, svd, qr #24

merged 14 commits into from
Feb 11, 2021

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented May 13, 2019

This will close #12
or at least get closer to it.

Right now I just have lu! (which gives lu and det and logdet).
I thought it might be good the share now to get feedback on the design of how to represent the factorization types.
This needs tests.

@oxinabox
Copy link
Member Author

oxinabox commented May 13, 2019

The same trick can be done for SVD, but it is a bit gross,
as it will attach the same labels to U as to Vt,
which will be wrong for both of them
but then during getproperty we can correct them by inserting the wildcards as required.

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #24 (d54b9cd) into master (77eafa0) will decrease coverage by 0.05%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   94.79%   94.74%   -0.06%     
==========================================
  Files          11       12       +1     
  Lines         423      476      +53     
==========================================
+ Hits          401      451      +50     
- Misses         22       25       +3     
Impacted Files Coverage Δ
src/NamedDims.jl 100.00% <ø> (ø)
src/functions_linearalgebra.jl 94.33% <94.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77eafa0...d6180f7. Read the comment docs.

@oxinabox
Copy link
Member Author

oxinabox commented Jun 4, 2019

The fact that we often store the dimension names via settign the type of factors or some arbitary field of the factorization is kinda hacky.
It will cause things to break hilariously if anyone uses getfield to access the fields of the Factorization.

I think it is fine, because LinearAlgegra basically accesses the underlying fields still via getproperty
and we always either strip the names or return the correct names from getproperty.
Infact we might even be able to do better by adding names to things that don't normally have them in getproperty to make more underling math work out.
even more danger-zone there though.

Alternative might be to have a NamedFactorization type, or something like that

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting to agree this is a kinda cool idea 😄

# Identity operation should give back original names
@test names(x.Q * x.R) == (:foo, :bar)

pivot && @testset "pivoted" begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sneaky! 😹

test/functions_linearalgebra.jl Outdated Show resolved Hide resolved
test/functions_linearalgebra.jl Outdated Show resolved Hide resolved
test/functions_linearalgebra.jl Outdated Show resolved Hide resolved
@oxinabox oxinabox changed the title Add LinearAlgebra Add LinearAlgebra: lu, lq, svd, qr Feb 2, 2020
@oxinabox
Copy link
Member Author

oxinabox commented Feb 2, 2020

I should rebase this, and address the comments on the PR.
If i had time...

@rofinn
Copy link
Member

rofinn commented Oct 21, 2020

@oxinabox If you wanna rebase your branch I can work on addressing the comments. I think making a NamedFactorization type makes sense to me.

@oxinabox
Copy link
Member Author

I will rebase this tomorrow then and you can take it over.

@oxinabox
Copy link
Member Author

@rofinn done. That was a weirdly hard rebase

@rofinn
Copy link
Member

rofinn commented Nov 17, 2020

Alright, now that my changes have been merged back I'm happy to approve, but maybe @nickrobinson251 wants to give it one more look over?

@rofinn
Copy link
Member

rofinn commented Jan 20, 2021

Should we just merge this as is and then I can update AxisKeys.jl?

@oxinabox
Copy link
Member Author

oxinabox commented Jan 20, 2021

yes?
I thought you had taken over this PR.
Oh were you waiting for @nickrobinson251 ?

@rofinn
Copy link
Member

rofinn commented Jan 20, 2021

Yeah, cause Nick was the original reviewer for this PR.

@nickrobinson251
Copy link
Contributor

Oh sorry I wasn't aware this was waiting on me. I'll take a look tomorrow :)

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

One question about tests for iterate (but i might just have missed them)

test/functions_linearalgebra.jl Show resolved Hide resolved
@oxinabox oxinabox merged commit a645914 into master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unary LinearAlgebra operations
3 participants