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.

Sign up for GitHub

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

fix Aqua's reported piracies and method ambiguities #85

Merged
merged 2 commits into from
Nov 21, 2024
Merged

fix Aqua's reported piracies and method ambiguities #85

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e9dfb7
fix Aqua's reported piracies and method ambiguities
Omar-Elrefaei Oct 19, 2024
9a814a5
test for piracies within our ecosystem that Aqua won't detect
Omar-Elrefaei Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/QSymbolicsBase/basic_ops_homogeneous.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ arguments(x::SScaled) = [x.coeff,x.obj]
operation(x::SScaled) = *
head(x::SScaled) = :*
children(x::SScaled) = [:*,x.coeff,x.obj]
function Base.:(*)(c, x::Symbolic{T}) where {T<:QObj}
function Base.:(*)(c::U, x::Symbolic{T}) where {U<:Union{Number, Symbolic{<:Number}},T<:QObj}
if (isa(c, Number) && iszero(c)) || iszero(x)
SZero{T}()
elseif _isone(c)
Expand All @@ -40,9 +40,9 @@ function Base.:(*)(c, x::Symbolic{T}) where {T<:QObj}
SScaled{T}(c, x)
end
end
Base.:(*)(x::Symbolic{T}, c) where {T<:QObj} = c*x
Base.:(*)(x::Symbolic{T}, c::Number) where {T<:QObj} = c*x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Motivation here was to avoid ambiguities with other packages,

But I'm not sure if it should be ::Number or ::{Union{Number, Symbolic{Number}}} to allow p*tr(q) just as tr(q)*p is allowed. The test suits for QuantumSymbolics and QuantumSavory pass right now, but I wasn't sure if that is a valid operation since it was allowed before my changes.

Base.:(*)(x::Symbolic{T}, y::Symbolic{S}) where {T<:QObj,S<:QObj} = throw(ArgumentError("multiplication between $(typeof(x)) and $(typeof(y)) is not defined; maybe you are looking for a tensor product `tensor`"))
Base.:(/)(x::Symbolic{T}, c) where {T<:QObj} = iszero(c) ? throw(DomainError(c,"cannot divide QSymbolics expressions by zero")) : (1/c)*x
Base.:(/)(x::Symbolic{T}, c::Number) where {T<:QObj} = iszero(c) ? throw(DomainError(c,"cannot divide QSymbolics expressions by zero")) : (1/c)*x
basis(x::SScaled) = basis(x.obj)

const SScaledKet = SScaled{AbstractKet}
Expand Down Expand Up @@ -94,13 +94,13 @@ arguments(x::SAdd) = x._arguments_precomputed
operation(x::SAdd) = +
head(x::SAdd) = :+
children(x::SAdd) = [:+; x._arguments_precomputed]
function Base.:(+)(xs::Vararg{Symbolic{T},N}) where {T<:QObj,N}
function Base.:(+)(x::Symbolic{T}, xs::Vararg{Symbolic{T}, N}) where {T<:QObj, N}
xs = (x, xs...)
xs = collect(xs)
f = first(xs)
nonzero_terms = filter!(x->!iszero(x),xs)
isempty(nonzero_terms) ? f : SAdd{T}(countmap_flatten(nonzero_terms, SScaled{T}))
end
Base.:(+)(xs::Vararg{Symbolic{<:QObj},0}) = 0 # to avoid undefined type parameters issue in the above method
basis(x::SAdd) = basis(first(x.dict).first)

const SAddBra = SAdd{AbstractBra}
Expand Down Expand Up @@ -137,7 +137,8 @@ arguments(x::SMulOperator) = x.terms
operation(x::SMulOperator) = *
head(x::SMulOperator) = :*
children(x::SMulOperator) = [:*;x.terms]
function Base.:(*)(xs::Symbolic{AbstractOperator}...)
function Base.:(*)(x::Symbolic{AbstractOperator}, xs::Vararg{Symbolic{AbstractOperator}, N}) where {N}
xs = (x, xs...)
zero_ind = findfirst(x->iszero(x), xs)
if isnothing(zero_ind)
if any(x->!(samebases(basis(x),basis(first(xs)))),xs)
Expand Down
2 changes: 2 additions & 0 deletions src/QSymbolicsBase/basic_superops.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
basis(x::KrausRepr) = basis(first(x.krausops))
Base.:(*)(sop::KrausRepr, op::Symbolic{AbstractOperator}) = (+)((i*op*dagger(i) for i in sop.krausops)...)
Base.:(*)(sop::KrausRepr, k::Symbolic{AbstractKet}) = (+)((i*SProjector(k)*dagger(i) for i in sop.krausops)...)
Base.:(*)(sop::KrausRepr, k::SZeroOperator) = SZeroOperator()
Base.:(*)(sop::KrausRepr, k::SZeroKet) = SZeroOperator()

Check warning on line 33 in src/QSymbolicsBase/basic_superops.jl

View check run for this annotation

Codecov / codecov/patch

src/QSymbolicsBase/basic_superops.jl#L32-L33 

Added lines #L32 - L33 were not covered by tests
Base.show(io::IO, x::KrausRepr) = print(io, "𝒦("*join([symbollabel(i) for i in x.krausops], ",")*")")

##
Expand Down
6 changes: 4 additions & 2 deletions test/test_aqua.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
@testitem "Aqua" tags=[:aqua] begin
using Aqua
import QuantumInterface as QI
own_types = [QI.AbstractBra, QI.AbstractKet, QI.AbstractSuperOperator, QI.AbstractOperator]
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate why these need to be marked as own? I am not sure whether this would be consistent with how independent packages downstream of QuantumInterface (e.g. QuantumOptics or QuantumCummulants) -- I want to make sure there are some precautions put in place for potential future issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, a type pyracy is when we expand a function definition with methods that take a certain type, but we neither defined the original function nor the type.
That was occurring all the time in files such as basic_ops_homogeneous.jl, basic_ops_inhomogeneous.jl, basic_superops.jl with methods that extend Base.(*) or Base.(+)

The last paragraph in the piracy section here gave me the impression that this was indeed happening intentionally. QuantumInterface defines the interface that is then used in other packages. So we tell Aqua that QuantumInterface and QuantumSymbolics are really cousin packages (since we avoided extending Base in QuantumInterface by design)

Aqua docs stating the following too gave me assurance that this is right.

this is useful for testing packages that deliberately commit some type piracies

Note: the functions really take Symbolic{T} not T directly, this seems to be worked around somewhere (same with Vector{T}) such that piracy detection is not tripped by these simple wrapper types.

Aqua.test_all(QuantumSymbolics,
ambiguities=(;broken=true),
piracies=(;broken=true),
ambiguities=(),
piracies=(;treat_as_own=own_types),
)
end
Loading