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

Addresses #46 and fixes #47 #50

Merged
merged 13 commits into from
Jun 21, 2024
Merged

Addresses #46 and fixes #47 #50

merged 13 commits into from
Jun 21, 2024

Conversation

apkille
Copy link
Member

@apkille apkille commented Jun 20, 2024

Added zero structs (#46) and created a hashing method for expressions containing quantum objects #47 in order to define expression equality for commutative operations.

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 7 lines in your changes missing coverage. Please review.

Project coverage is 65.86%. Comparing base (237d158) to head (076399a).

Files Patch % Lines
src/QSymbolicsBase/QSymbolicsBase.jl 60.00% 2 Missing ⚠️
src/QSymbolicsBase/basic_ops_inhomogeneous.jl 86.66% 2 Missing ⚠️
src/QSymbolicsBase/literal_objects.jl 75.00% 2 Missing ⚠️
src/QSymbolicsBase/basic_ops_homogeneous.jl 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   62.62%   65.86%   +3.23%     
==========================================
  Files          15       15              
  Lines         594      621      +27     
==========================================
+ Hits          372      409      +37     
+ Misses        222      212      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Wonderful job! I have mostly stylistic/idioms comments. But I think I also found a place where we have introduced a small bug previously.

src/QSymbolicsBase/QSymbolicsBase.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/basic_ops_homogeneous.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/QSymbolicsBase.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/basic_ops_homogeneous.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/basic_ops_homogeneous.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/basic_ops_homogeneous.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/basic_ops_homogeneous.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/literal_objects.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/literal_objects.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/predefined.jl Outdated Show resolved Hide resolved
@Krastanov
Copy link
Member

A day or two ago I spent some time to make sure that all tests pass across this github organization, so I will be a bit more strict on tests during review. Here is a list of explanations:

The macos error was just a random happenstance

The codecov coverage issue is not great, but not a blocker

The performance tracking is not set up yet, that is why it is failing

The changelog will be updated on merging

The only problematic one is the JET tests. JET is a static analyzer, i.e. it attempts to detect bugs without running the code, i.e. logic bugs that would be pretty difficult to detect when running the code anyway because they might be in an edge case not covered by tests and rarely encountered by users. It does have false positives though, so I have simply upper bounded the number of errors it is permitted to detect. It was at 6, but now it detects 7 issues. Could you check if any of these issues were caused by something changed in this PR. I copy them here for convenience:

 rep = ═════ 7 possible errors found ═════
┌ commutator(o1::SymbolicUtils.Symbolic{…}, o2::SymbolicUtils.Symbolic{…}) @ QuantumSymbolics /home/runner/work/QuantumSymbolics.jl/QuantumSymbolics.jl/src/QSymbolicsBase/basic_ops_homogeneous.jl:209
│┌ QuantumSymbolics.SCommutator(o1::SymbolicUtils.Symbolic{…}, o2::SymbolicUtils.Symbolic{…}) @ QuantumSymbolics /home/runner/work/QuantumSymbolics.jl/QuantumSymbolics.jl/src/QSymbolicsBase/basic_ops_homogeneous.jl:199
││┌ hcat(::SymbolicUtils.Symbolic{…}, ::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1944
│││┌ kwcall(::@NamedTuple{…}, ::typeof(cat), ::SymbolicUtils.Symbolic{…}, ::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1993
││││┌ cat(::SymbolicUtils.Symbolic{…}, ::SymbolicUtils.Symbolic{…}; dims::Val{…}) @ Base ./abstractarray.jl:1993
│││││┌ _cat(::Val{…}, ::SymbolicUtils.Symbolic{…}, ::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1796
││││││┌ _cat_t(::Val{…}, ::Type{…}, ::SymbolicUtils.Symbolic{…}, ::SymbolicUtils.Symbolic{…}) where T @ Base ./abstractarray.jl:1805
│││││││┌ __cat(::Matrix, ::Tuple{…}, ::Tuple{…}, ::SymbolicUtils.Symbolic{…}, ::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1812
││││││││┌ __cat_offset!(A::Matrix, shape::Tuple{…}, catdims::Tuple{…}, offsets::Tuple{…}, x::SymbolicUtils.Symbolic{…}, X::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1816
│││││││││┌ __cat_offset1!(A::Matrix, shape::Tuple{…}, catdims::Tuple{…}, offsets::Tuple{…}, x::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1825
││││││││││┌ _copy_or_fill!(A::Matrix, inds::Tuple{…}, x::SymbolicUtils.Symbolic{…}) @ Base ./abstractarray.jl:1832
│││││││││││┌ fill!(V::SubArray{Bool, 2, Union{}, Tuple{…}, false}, x::SymbolicUtils.Symbolic{QuantumInterface.AbstractOperator}) @ Base ./multidimensional.jl:1512
││││││││││││┌ getproperty(x::SubArray{Bool, 2, Union{}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}, f::Symbol) @ Base ./Base.jl:37
│││││││││││││ invalid builtin function call: Base.getfield(x::SubArray{Bool, 2, Union{}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}, f::Symbol)
││││││││││││└────────────────────
│││││││││││┌ fill!(V::SubArray{…}, x::SymbolicUtils.Symbolic{…}) where Tv @ SparseArrays /cache/build/builder-amdci4-0/julialang/julia-release-1-dot-10/usr/share/julia/stdlib/v1.10/SparseArrays/src/sparsematrix.jl:3168
││││││││││││┌ getproperty(x::SubArray{Tv, 2, Union{}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false} where Tv, f::Symbol) @ Base ./Base.jl:37
│││││││││││││ invalid builtin function call: Base.getfield(x::SubArray{Tv, 2, Union{}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false} where Tv, f::Symbol)
││││││││││││└────────────────────
┌  @ QuantumSymbolics /home/runner/work/QuantumSymbolics.jl/QuantumSymbolics.jl/src/QSymbolicsBase/predefined.jl:314
│ no matching method found `QuantumSymbolics.SMulOperator(::QuantumSymbolics.SInvOperator, ::QuantumSymbolics.SOperator)`: SMulOperator(invop::QuantumSymbolics.SInvOperator, op::QuantumSymbolics.SOperator)
└────────────────────
┌  @ QuantumSymbolics /home/runner/work/QuantumSymbolics.jl/QuantumSymbolics.jl/src/QSymbolicsBase/predefined.jl:315
│ no matching method found `QuantumSymbolics.SMulOperator(::QuantumSymbolics.SOperator, ::QuantumSymbolics.SInvOperator)`: SMulOperator(op::QuantumSymbolics.SOperator, invop::QuantumSymbolics.SInvOperator)
└────────────────────
┌ (::QuantumSymbolics.var"#43#45")(__MATCHES__::Any) @ QuantumSymbolics /home/runner/.julia/packages/SymbolicUtils/0opve/src/rule.jl:314
│ `QuantumSymbolics.xs` is not defined: QuantumSymbolics.prefactorscalings_rule(QuantumSymbolics.xs)
└────────────────────
┌ apply_recipe(x::QuantumSymbolics.SDagger; kwargs...) @ QuantumSymbolics /home/runner/work/QuantumSymbolics.jl/QuantumSymbolics.jl/src/QSymbolicsBase/latexify.jl:31
│┌ getproperty(x::QuantumSymbolics.SDagger, f::Symbol) @ Base ./Base.jl:37
││ type SDagger has no field ket: Base.getfield(x::QuantumSymbolics.SDagger, f::Symbol)
│└────────────────────
┌ basis(x::QuantumSymbolics.StabilizerState) @ QuantumSymbolics /home/runner/work/QuantumSymbolics.jl/QuantumSymbolics.jl/src/extensions.jl:25
│┌ ^(b::SpinBasis{1//2, Int64}, N::Integer) @ QuantumInterface /home/runner/.julia/packages/QuantumInterface/VISSI/src/bases.jl:100
││┌ collect(itr::Base.Generator{_A, QuantumInterface.var"#3#4"{SpinBasis{1//2, Int64}}} where _A) @ Base ./array.jl:844
│││┌ collect_to_with_first!(dest::Any, v1::SpinBasis{…}, itr::Base.Generator{…} where _A, st::Any) @ Base ./array.jl:875
││││┌ grow_to!(dest::AbstractDict{…}, itr::Base.Generator{…} where _A, st::Any) where {…} @ Base ./dict.jl:133
│││││┌ indexed_iterate(I::SpinBasis{1//2, Int64}, i::Int64) @ Base ./tuple.jl:95
││││││ no matching method found `iterate(::SpinBasis{1//2, Int64})`: x = iterate(I::SpinBasis{1//2, Int64})
│││││└────────────────────

@Krastanov
Copy link
Member

actually, it seems the JET tests fail on master too, because of something I must have merged last minute as I was cleaning up the tests. You can just bump the threshold to 7 and not bother with them

@Krastanov
Copy link
Member

Looks good, feel free to merge after you add a changelog and all tests except "performance tracking" and "codecov project" pass. Some more details on merging etiquette were posted in the other PR

@apkille apkille dismissed Krastanov’s stale review June 21, 2024 17:02

Permission to merge

@apkille apkille merged commit 0f47b58 into QuantumSavory:main Jun 21, 2024
16 of 17 checks passed
@apkille apkille deleted the newstructs branch June 21, 2024 17:07
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