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

Bug fix to StackOverflowError for md ⊗ S"X" where typeof(md) is MixedDestabilizer{...} #354

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Fe-r-oz
Copy link
Contributor

@Fe-r-oz Fe-r-oz commented Sep 7, 2024

With this fix, sm ⊗ S"X" from #260 will be added without any worry of StackOverflow error.

  • The code is properly formatted and commented.
  • Substantial new functionality is documented within the docs.
  • All new functionality is tested.
  • All of the automated tests on github pass.

P.S. Pondering about more tests for this.

Local Output w.r.t Task 4.

     Testing Running tests...
skipping gpu tests (set GPU_TESTS=true to test gpu)
Starting tests with 1 threads out of `Sys.CPU_THREADS = 4`...
[ Info: SetupBuildDirectory: setting up build directory.
[ Info: Doctest: running doctests.
[ Info: Skipped ExpandTemplates step (doctest only).
[ Info: Skipped CrossReferences step (doctest only).
[ Info: Skipped CheckDocument step (doctest only).
[ Info: Skipped Populate step (doctest only).
[ Info: Skipped RenderDocument step (doctest only).
WARNING: using LinearAlgebra.I in module QuantumClifford conflicts with an existing identifier.
Skipping Base.active_repl
Skipping Base.active_repl_backend
Test Summary: |   Pass   Total     Time
Package       | 285936  285936  7m19.9s
     Testing QuantumClifford tests passed 

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Sep 7, 2024

Tasks Completed:

  • MixedDestabilizer ⊗ Stabilizer: md ⊗ S"X"
  • Stabilizer ⊗ MixedDestabilizer: S"X" ⊗ md

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Sep 8, 2024

I think it is correct to say the resulting MixedDestabilizer resulting from md ⊗ S"X" should pass the mixed_destab_looks_good to ensure a consistent MixedDestabilizer. I have added tests for this to hold.

Copy link
Member

@Krastanov Krastanov left a 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.

src/linalg.jl Show resolved Hide resolved
src/linalg.jl Outdated Show resolved Hide resolved
@Fe-r-oz Fe-r-oz marked this pull request as draft September 14, 2024 06:25
@Fe-r-oz Fe-r-oz marked this pull request as ready for review September 14, 2024 22:51
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Sep 14, 2024

Thank you, promote_rule was very helpful. Addressed all of your comments.

I think the PR is ready for review.

P.S. Pardon for last 1 commit :( , applied further polishing.

Copy link
Member

@Krastanov Krastanov left a 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 Show resolved Hide resolved
Comment on lines 996 to 1003
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}
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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/linalg.jl Outdated Show resolved Hide resolved
Comment on lines 1005 to 1012
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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these converts 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.

@Fe-r-oz Fe-r-oz marked this pull request as draft September 15, 2024 15:50
@Fe-r-oz Fe-r-oz marked this pull request as ready for review September 17, 2024 13:30
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Sep 17, 2024

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 Base.convert(::Type{S}, x::AbstractStabilizer) where {S <: AbstractStabilizer} = S(x.tab) because the MixedStab and MixedDestab requires rank parameter as well. I simplified convert following the spirit of simplification as well.

Hopefully, this attempt is satisfactory and less awkward.

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.

2 participants