reduce JET errors #364
reduce JET errors #364
Conversation
|
There were two errors that were not appearing in the local JET: related to I think the PR is ready for review. Thank you! |
Krastanov
added
the
Skip-Changelog
There was a problem hiding this comment.
Thank you for spending time on these. Reducing the JET warnings is very valuable, but I am not very comfortable with these changes. One of them introduces a bug and the other one makes the code much less readable. I would say that these are issues that should be fixed in JET itself or a more lower-level fix is necessary (e.g. in Base). Because of this, I will close this PR for now.
It is pretty frustrating and timeconsuming to search for these warnings, I know -- this close to Base it is difficult to make heads or tails of JET reports. I appreciate you attempted these fixes.
| @@ -34,7 +34,7 @@ function petrajectory(state, circuit; branch_weight=1.0, current_order=0, max_or | |||
| if status==continue_stat # TODO is the copy below necessary? | |||
| out_probs = petrajectory(copy(newstate), rest_of_circuit, | |||
| branch_weight=branch_weight*prob, current_order=current_order+order, max_order=max_order) | |||
| status_probs .+= out_probs | |||
There was a problem hiding this comment.
I am not a big fan of this change -- it is great that it fixes a JET issue, but it makes the code much harder to read. Without a better understanding of why this is an issue, I prefer to keep this as it is.
| fastrow(t::Tableau{Tₚᵥ,Tₘ}) where {Tₚᵥ, Tₘ<:Adjoint} = Tableau(t.phases, t.nqubits, collect(t.xzs)) | ||
| fastcolumn(t::Tableau{Tₚᵥ,Tₘ}) where {Tₚᵥ, Tₘ} = Tableau(t.phases, t.nqubits, collect(t.xzs')') | ||
| fastrow(t::Tableau{Tₚᵥ,Tₘ}) where {Tₚᵥ, Tₘ<:Adjoint} = Tableau(t.phases, t.nqubits, Matrix(collect(t.xzs))) | ||
| fastcolumn(t::Tableau{Tₚᵥ,Tₘ}) where {Tₚᵥ, Tₘ} = Tableau(t.phases, t.nqubits, Matrix(collect(t.xzs')')) |
There was a problem hiding this comment.
I think this introduces a bug:
julia> typeof(a)
Matrix{Float64} (alias for Array{Float64, 2})
julia> typeof(a')
LinearAlgebra.Adjoint{Float64, Matrix{Float64}}
julia> typeof(Matrix(a'))
Matrix{Float64} # we want it to be an Adjoint view
It undoes the purposeful change to the memory layout. Sure, the results you get by indexing into it are the same, but we specifically want to preserve the column-major or row-major order
There was a problem hiding this comment.
Thanks for all these insightful comments.
|
if you are still interested in trying to pursue these tasks (which I should warn you, it will probably be very time consuming work with limited gains), I would suggest looking into what in Base causes the issues. |
This PR aims to reduce JET errors a bit further.