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

Fixing equality for metadata-decorated composite types #62

Closed
apkille opened this issue Jul 9, 2024 · 4 comments
Closed

Fixing equality for metadata-decorated composite types #62

apkille opened this issue Jul 9, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@apkille
Copy link
Member

apkille commented Jul 9, 2024

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
@apkille apkille added the bug Something isn't working label Jul 9, 2024
@Krastanov
Copy link
Member

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
Copy link
Member Author

apkille commented Jul 9, 2024

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
Copy link
Member

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
Copy link
Member Author

apkille commented Jul 9, 2024

OK, that sounds good.

@apkille apkille mentioned this issue Jul 9, 2024
apkille added a commit that referenced this issue Jul 12, 2024
* skip metadata in propsequal

* add tests and update changelog, project.toml

* update changelog
@apkille apkille closed this as completed Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants