Conversation
|
The tests pass on macOS + Julia 1.12 locally for me... 😱 |
| jpvt = Vector{Int}(undef, 0) | ||
| τ = Vector{eltype(A)}(undef, 0) |
There was a problem hiding this comment.
What is the least impactful: making these nothing and having a small type union, or the current approach, which I think does require a small allocation?
| G = V' * V | ||
|
|
||
| VᴴΔV = !iszerotangent(ΔV) ? V' * ΔV : zero(G) | ||
| ΔVperp = ΔV - V * inv(G) * VᴴΔV |
There was a problem hiding this comment.
Does this even work if iszerotangent(ΔV), e.g. ΔV could be nothing on some of the AD engines like Zygote, no?
| indU = axes(U, 2) | ||
| indV = axes(Vᴴ, 1) |
There was a problem hiding this comment.
These can probably immediately get their final value, since ind is always defined, even if we end up not needing them.
| for i in 1:2 # first call returns lwork as work[1] | ||
| #! format: off | ||
| if eltype(A) <: Complex | ||
| rwork_ = isnothing(rwork) ? Vector{$relty}(undef, 0) : rwork |
There was a problem hiding this comment.
I don't see how this one helps? if eltype(A) <: Complex, then also rwork is not nothing. Is it just a matter of trying to show this to the compiler? How about
rwork_::Vector{$relty} = rwork?
There was a problem hiding this comment.
if eltype(A) <: Complex, then also rwork is not nothing
JET seems unable to deduce this fact :(
Let's try the suggestion and see how it reacts!
There was a problem hiding this comment.
Nope, no dice, because from JET's PoV this could still involve a conversion from Nothing to a Vector{$relty}...
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I don't have anything against making JET happy by inserting things that don't matter in the end, but I am incredibly annoyed by the idea of having to introduce (small) type instabilities or allocations just to make a linter happy. On that note, I would actually be more inclined to simply turn off JET tests rather than trying to keep working around this. On top of that, these days for me it seems the JET tests on 1.12 seem to fail often because of errors rather than test failures, so even if we work around it, it still gives red X's too often 😛
|
It's also a real pain that I see everything passing locally but it fails in |
|
Then let's just disable JET for now, we can revisit this in the future if the need arises. |
3 participants
I know @lkdvos has said he dislikes these ticky-tacky small changes to make the linter happy, which I am very sympathetic to. On the other hand, I dislike red "x"s in CI. Happy to modify this though (please just feel free to push stuff!).