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.
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
Ad extension [WIP] #85
Ad extension [WIP] #85
Changes from 11 commits
1eccb72
3cceb8d
7e2a116
a20d41c
ac5f08a
bdf7a97
8f655e6
4e8e1fa
c4f6a48
6213881
2395293
d7d745e
e703eef
387d9dd
06a36a6
55a49a3
3441981
18a042a
45e7d7c
0956858
503b536
f32f4f0
b60adc5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 44 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L43-L44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a type instability (I assume this is what you were referring to this week?)
Do you think we can avoid this by:
pullback_eigsolve(ΔX::Tuple{AbstractZero, AbstractZero, Any}) = [...]
(with∂f = ZeroTangent()
)I am honestly not sure if the second case ever happens, as I think this implies that the dependence on the eigenvalues and eigenvectors is exactly zero, which sounds incredibly implausible with floating point accuracy. This would both mean that the regular (most common) case is now type-stable, and that the case where
n = 0
gets handled properly when both inputs areAbstractZero
(which afaik e.g. Zygote would never even generate either)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will get rid of this.
Check warning on line 47 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L47
Check warning on line 53 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L53
Check warning on line 79 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L78-L79
Check warning on line 83 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L82-L83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that still has to be added? Do we have to check that
eltype(A)
is real, or do you think we can get away with just assuming that exact conjugate pairs only ever appear in that context?conj(vec)
is definitely not something that is part of our current interface for vectors, so in that sense it is also not ideal...In principle I have nothing against just explicitly adding
if f isa Matrix{<:Real} && i > 1
or something along these lines. I think the compiler is smart enough to just elide that whenever appropriateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would indeed work.
Check warning on line 99 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L99
Check warning on line 104 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L104
Check warning on line 119 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L119
Check warning on line 121 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L121
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check warning on line 154 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L154
Check warning on line 210 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L210
Check warning on line 224 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L224
Check warning on line 232 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L232
Check warning on line 239 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L234-L239
Check warning on line 242 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L241-L242
Check warning on line 251 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L245-L251
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about avoiding computing the check when the verbosity is low
Check warning on line 256 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L253-L256
Check warning on line 265 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L258-L265
Check warning on line 275 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L268-L275
Check warning on line 278 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L277-L278
Check warning on line 284 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L280-L284
Check warning on line 286 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L286
Check warning on line 294 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L288-L294
Check warning on line 299 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L296-L299
Check warning on line 303 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L302-L303
Check warning on line 317 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L307-L317
Check warning on line 321 in ext/KrylovKitChainRulesCoreExt/eigsolve.jl
Codecov / codecov/patch
ext/KrylovKitChainRulesCoreExt/eigsolve.jl#L319-L321