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

Restrict ldiv! rules to Cholesky #1257

Merged
merged 5 commits into from
Jan 29, 2024
Merged

Restrict ldiv! rules to Cholesky #1257

merged 5 commits into from
Jan 29, 2024

Conversation

devmotion
Copy link
Contributor

I noticed that the ldiv! rules added in #1220 also support Array arguments (or Const etc. of Arrays). However, no such primal methods exist in LinearAlgebra:

julia> using LinearAlgebra

julia> methods(ldiv!, Tuple{Array, Any})
# 0 methods for generic function "ldiv!" from LinearAlgebra

There exist a few methods for AbstractMatrix subtypes such as Diagonal but I think it might be best to just restrict the rule to Cholesky for now (also in line with the docstring of ldiv! which tells users that "The argument A should not be a matrix. Rather, instead of matrices it should be a factorization object").

Additionally, the PR fixes a call to the primal in the reverse rule which did not forward the kwargs... correctly.

@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3095a4f) 74.66% compared to head (888d47c) 74.93%.

Files Patch % Lines
src/internal_rules.jl 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1257      +/-   ##
==========================================
+ Coverage   74.66%   74.93%   +0.26%     
==========================================
  Files          34       35       +1     
  Lines       10300    10310      +10     
==========================================
+ Hits         7691     7726      +35     
+ Misses       2609     2584      -25     

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

src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
src/internal_rules.jl Outdated Show resolved Hide resolved
@wsmoses wsmoses merged commit d4f6400 into EnzymeAD:main Jan 29, 2024
40 of 43 checks passed
@devmotion devmotion deleted the patch-1 branch January 29, 2024 01:17
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.

3 participants