search

QuantumSavory / QuantumSymbolics.jl

Computer algebra tools for symbolic manipulations in quantum mechanics and quantum information
MIT License
29 stars 9 forks source link

Fixing equality for metadata-decorated composite types #62

Closed apkille closed 1 month ago

apkille commented 2 months ago

Describe the bug 🐞

The macro @withmetadata adds a metadata property to a defined struct. This becomes an issue when propsequal is called because this function evaluates equality of two symbolic objects by evaluating the equality of their properties. But we shouldn't care about metadata when evaluating equality, as that only contains information about the cached results from express.

Minimal Reproducible Example 👇

julia> state1 = XBasisState(1, SpinBasis(1//2))
|X₁⟩

julia> state2 = XBasisState(1, SpinBasis(1//2))
|X₁⟩

julia> isequal(state1, state2)
false

julia> QuantumSymbolics.propsequal(state1, state2)
false

julia> i1, b1, m1 = propertynames(state1)
(:idx, :basis, :metadata)

julia> i2, b2, m2 = propertynames(state2)
(:idx, :basis, :metadata)

julia> isequal(getproperty(state1, m1), getproperty(state2, m2))
false
Krastanov commented 2 months ago

Is it sufficient to just skip :metadata in propsequal?

Also, jeez, how did this not pop up in the past... It looks like a pretty bad issue.

apkille commented 2 months ago

Is it sufficient to just skip :metadata in propsequal?

That would be the most straightforward fix IMO, although I wasn't sure if this is the most efficient fix and whether or not we should consider a better way of evaluating equality. Perhaps this is OK for now. This is somewhat related, but it might also be good to clean up the withmetadata macro in the process, as it's pretty ugly at the moment. We could maybe use the capture features from MacroTools.jl or MLStyle.jl. Let me know your thoughts.

Also, jeez, how did this not pop up in the past... It looks like a pretty bad issue.

I was surprised about this too, but it's most likely because the past few PRs have not dealt with predefined objects like basis states, which is how I encountered the issue.

Krastanov commented 2 months ago

I would suggest a separate PR for the simple fix for this issue, which can be merged quickly, and then potentially a future PR that simplifies the macro, if that is reasonably easy to do with the tools you had suggested. There is also https://github.com/Roger-luo/Expronicon.jl which can be useful for this type of work.

apkille commented 2 months ago

OK, that sounds good.