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 eigen and eigvals rules for StridedMatrix #321

Merged
merged 24 commits into from
Dec 9, 2020

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Dec 4, 2020

This PR adds rules for eigen and eigvals. It is very conservative and only applies for StridedMatrixes. Because the normalization and phase convention for eigen for these matrices is known, the rules can explicitly account for this, which allows us to stably test with FiniteDifferences.

A few notes:

  • The rule doesn't handle degenerate matrices correctly. This can perhaos be handled in a future PR with the approach mentioned in Backpropagation-Friendly Eigendecomposition #144 (perhaps in [WIP] rrule for eigen decomposition  #179 or a follow-up). The methods that handle the normalization/phase contribution could be directly applied to that rule.
  • The rule doesn't do the right thing for non-diagonal hermitian matrices, as those dispatch to eigen(::Hermitian), which uses a different phase convention. That is, the rule will work in a program that is invariant to the phase convention but not one that isn't. This will be handled in a future PR for eigen(::Hermitian) (currently defined in Symmetric/Hermitian matrix function rules #193 but will be split into its own PR)

@willtebbutt
Copy link
Member

This is great.

Would you mind adding some @inferred tests? I've recently come to realise that one source of performance issues in Zygote is type-instabilities in the primitives, so it would be good to start trying to clamp down on that.

@sethaxen
Copy link
Member Author

sethaxen commented Dec 4, 2020

Would you mind adding some @inferred tests? I've recently come to realise that one source of performance issues in Zygote is type-instabilities in the primitives, so it would be good to start trying to clamp down on that.

I can do that. Perhaps it would be good to add this to the testers in ChainRulesTestUtils with a configurable option like check_inferred, on by default.

@willtebbutt
Copy link
Member

I can do that. Perhaps it would be good to add this to the testers in ChainRulesTestUtils with a configurable option like check_inferred, on by default.

Yes. This would be an excellent idea.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

I'm happy with this, modulo a patch bump.

@codecov-io
Copy link

codecov-io commented Dec 5, 2020

Codecov Report

Merging #321 (6a519a4) into master (80443b1) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
+ Coverage   97.43%   97.62%   +0.18%     
==========================================
  Files          18       18              
  Lines         937     1011      +74     
==========================================
+ Hits          913      987      +74     
  Misses         24       24              
Impacted Files Coverage Δ
src/rulesets/LinearAlgebra/factorization.jl 99.33% <100.00%> (+0.63%) ⬆️

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 80443b1...6a519a4. Read the comment docs.

@sethaxen
Copy link
Member Author

sethaxen commented Dec 5, 2020

Thanks! All tests pass so I'll leave open for a few days to give others a chance to review and then merge on Tuesday.

@sethaxen
Copy link
Member Author

sethaxen commented Dec 6, 2020

I changed the frules to target the more basic mutating eigen! and eigvals!, so the test suite now needs JuliaDiff/ChainRulesTestUtils.jl#79 to pass, and this should be re-reviewed.

sethaxen added a commit to sethaxen/ChainRules.jl that referenced this pull request Dec 6, 2020
@oxinabox oxinabox requested a review from willtebbutt December 7, 2020 14:53
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

This looks great, and should be merged with a patch bump.

@sethaxen sethaxen merged commit 22eddb4 into JuliaDiff:master Dec 9, 2020
@sethaxen sethaxen deleted the eigen branch December 9, 2020 22:29
@nickrobinson251 nickrobinson251 added the type constraints Potentially raises a question about how tightly to constrain argument types for a rule. See #232 label Jan 1, 2021
sethaxen added a commit that referenced this pull request Jan 7, 2021
* Move symmetric rules to own file

* Move symmetric tests to own file

* Adapt eigen and eigvals rules from #321

* Don't allocate

* Implement mutating forms for frule

* Add sortby keyword

* Use fewer indices

* Correctly reference BlasReal

* Move eigen pullback to external function

* Add hermitian svd rrule

* Hermitrize in pullback

* Separate out eigvals sign code

* Add svdvals rrule

* Rearrange functions

* Realify eigenvalue cotangents

* Restrict to StridedMatrixes

* Reduce allocations and unnecessary ops

* Avoid unnecessary allocation in svd

* Explicitly create eigvals pullback inputs

* Simplify _hermitrize!

* Add svdvals section

* Don't use convenience type not in v1.0

* Fix multiplication order

* Remove ambiguity in signature

* Define missing variable

* Make pure imaginary

* Add tests for eigendecomposition rules

* Test from nonsymmetric matrix

* Add newlines

* Remove unnecessary unthunks

* Test type-stability

* Test mixtures of Zeros

* Use more informative testset names

* Fix svd pullback bugs

* Add svd pullback tests

* Return correct argument

* Remove unused (co)tangents

* Add svdvals tests

* Fix typo

* Restrict SVD test to greater than v1.3.0

* Only check type-stability on 1.6

* Avoid specifying sortby keyword

This is not defined on earlier Julia versions

* Remove obsolete comment

* Abandon ship when derivatives explode

* Handle Hermitian special-case for general eigen

* Fix comment

* Resolve type-instability

* Handle when just ΔV is Zero

* Test eigen for hermitian Matrix

* Make hermitian Matrix

* Call eigen pullback from eigvals

* Only pass sortby to Hermitian eigen

* Support Julia 1.0's return for _symherm_back

* Call Hermitian eigvals! frule

* Call eigen rrule in eigval rrule

* Test eigvals for hermitian Matrix-es

* Do less expensive eltype check first

* Correctly handle sortby default

* Increment version number

* Add references and notes

* More clearly name tests

* Add comment explaining test set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type constraints Potentially raises a question about how tightly to constrain argument types for a rule. See #232
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants