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

use set equality when checking for expression equality of commutative terms (and potentially generally use sets for commutative terms) #47

Closed
Krastanov opened this issue Jun 19, 2024 · 3 comments

Comments

@Krastanov
Copy link
Member

we already have

julia> Set([X1, X2]) == Set([X1, X2])
true

julia> Set([X1, X2]) == Set([X1, Z1])
false

Set uses hashing for quick (potentially false positive) equality check in inclusion (log(n)), followed by isequal check to avoid false positive. Thus set equalities have cost of n log(n) instead of n^2 without the use of a smart datastructure.

Initially there was a worry that Set uses ==, which would have broken its use with Symbolics (where == is used to create a symbolic equation), but that does not seem to be the case.

To avoid creating a new Set at every equality check, we should probably have it as part of the structure. E.g.

@withmetadata struct SAdd{T<:QObj} <: Symbolic{T}
    dict
    _set_precomputed
    _arguments_precomputed
end

It will take a bit more memory (not much given that the objects are not copied, only referenced), but it should speed up a lot of other code.

pinging @apkille as we had been discussing this in #40

@apkille
Copy link
Member

apkille commented Jun 20, 2024

Could you show me your source that explains how Set checks equality? I'm getting the following result:

julia> Set([dagger(X1)]) == Set([dagger(X1)])
false

julia> dagger(X1) == dagger(X1)
false

julia> isequal(dagger(X1), dagger(X1))
true

julia> X1 == X1
true

@apkille
Copy link
Member

apkille commented Jun 20, 2024

Ah, the hash changes each time dagger is called:

julia> hash(X1)
0x00671145608bd477

julia> hash(X1)
0x00671145608bd477

julia> hash(dagger(X1))
0x44e2b6a69704df53

julia> hash(dagger(X1))
0xb846fec1d33c803b

@Krastanov
Copy link
Member Author

I think a hash is not defined for these objects so it defaults to memory id as hash. Defining a hash function should take care of this (and they have a pretty simple standard definition in this case along the lines of hash(arguments(expr)) (but it should be the two-argument hash)).

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

No branches or pull requests

2 participants