-
Notifications
You must be signed in to change notification settings - Fork 47
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
Type promotion between MixedDestabilizer, Stabilizer and others, also used in ⊗ #354
Conversation
Tasks Completed:
|
I think it is correct to say the resulting |
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.
Half of this is on the correct path, but the other half presents complication we can avoid. Look into promote_rule
if you would like to complete this PR.
Thank you, I think the PR is ready for review. P.S. Pardon for last 1 commit :( , applied further polishing. |
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.
There are some conceptual errors here about how to convert between different representations -- they are pretty fundamental to what each of the types actually means.
I also suggested a few simplifications.
src/QuantumClifford.jl
Outdated
Base.promote_rule(::Type{<:MixedStabilizer{T}} , ::Type{<:Stabilizer{T}} ) where {T<:Tableau} = Stabilizer{T} | ||
Base.promote_rule(::Type{<:MixedStabilizer{T}} , ::Type{<:MixedStabilizer{T}}) where {T<:Tableau} = Stabilizer{T} | ||
Base.promote_rule(::Type{<:MixedStabilizer{T}} , ::Type{<:Destabilizer{T}} ) where {T<:Tableau} = MixedDestabilizer{T} | ||
Base.promote_rule(::Type{<:Stabilizer{T}} , ::Type{<:Destabilizer{T}} ) where {T<:Tableau} = MixedDestabilizer{T} | ||
Base.promote_rule(::Type{<:Destabilizer{T}} , ::Type{<:Destabilizer{T}} ) where {T<:Tableau} = MixedDestabilizer{T} | ||
Base.promote_rule(::Type{<:MixedDestabilizer{T}}, ::Type{<:Stabilizer{T}} ) where {T<:Tableau} = MixedDestabilizer{T} | ||
Base.promote_rule(::Type{<:MixedDestabilizer{T}}, ::Type{<:Destabilizer{T}} ) where {T<:Tableau} = MixedDestabilizer{T} | ||
Base.promote_rule(::Type{<:MixedDestabilizer{T}}, ::Type{<:MixedStabilizer{T}}) where {T<:Tableau} = MixedDestabilizer{T} |
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.
I do not agree with many of these rules. A MixedStabilizer can not be converted to Stabilizer.
From special to general the conversions would be (missing a lot of type parameterizations):
Stabilizer, T<:Union{MixedStabilizer, Destabilizer, MixedDestabilizer} -> T
Destabilizer, MixedDestabilizer -> MixedDestabilizer
MixedStabilizer, MixedDestabilizer -> MixedDestabilizer
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.
Thank you, I agree with you. I will remove the nonsensical rules. Also, incorporate your last comment about not missing the type parameterizations.
tensor(MixedStab(...), S"X")
causes stack overflow error if a user wants to cause it, so most of the nonsensical rules were to avoid the stack overflow errors. But yeah, they result in nonsense objects which I completely overlooked. Sorry about that.
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.
Thank you very much for special to general
conversions #354 (comment), I have added them as instructed.
src/QuantumClifford.jl
Outdated
Base.convert(::Type{<:Stabilizer{T}} , x::MixedStabilizer{T} ) where {T<:Tableau} = Stabilizer(tab(x)) | ||
Base.convert(::Type{<:MixedStabilizer{T}} , x::MixedStabilizer{T} ) where {T<:Tableau} = Stabilizer(tab(x)) | ||
Base.convert(::Type{<:Stabilizer{T}} , x::Destabilizer{T} ) where {T<:Tableau} = MixedDestabilizer(x) | ||
Base.convert(::Type{<:Destabilizer{T}} , x::Destabilizer{T} ) where {T<:Tableau} = MixedDestabilizer(x) | ||
Base.convert(::Type{<:MixedDestabilizer{T}} , x::Stabilizer{T} ) where {T<:Tableau} = MixedDestabilizer(x) | ||
Base.convert(::Type{<:MixedDestabilizer{T}} , x::Destabilizer{T} ) where {T<:Tableau} = MixedDestabilizer(x) | ||
Base.convert(::Type{<:Destabilizer{T}} , x::MixedStabilizer{T} ) where {T<:Tableau} = MixedDestabilizer(Stabilizer(tab(x))) | ||
Base.convert(::Type{<:MixedDestabilizer{T}} , x::MixedStabilizer{T} ) where {T<:Tableau} = MixedDestabilizer(Stabilizer(tab(x))) |
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.
All of these convert
s can be implemented as something much simpler, e.g. convert(::T, x::AbstractStabilizer) where {T<:Type{S}, S<:AbstractStabilizer} = S(x)
because we already have the constructors implemented. I probable screwed up some of the type syntax or names of types, but this is the rough idea.
Thank you, my previous attempt was just so bad... I have removed all the nonsense and incorporated your comments. Specifically, I have revamped the promote rules based of your description. I had some complication using Hopefully, this attempt is satisfactory and less awkward. |
md ⊗ S"X"
where typeof(md)
is MixedDestabilizer{...}
Thank you! |
With this fix,
sm ⊗ S"X"
from #260 will be added without any worry of StackOverflow error.P.S. Pondering about more tests for this.
Local Output w.r.t Task 4.