-
Notifications
You must be signed in to change notification settings - Fork 15
Cluster expansions #47
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
base: main
Are you sure you want to change the base?
Conversation
merge from hpc/clusters
Project.toml
Outdated
| Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e" | ||
| CUDA = "052768ef-5323-5732-b1bb-66c8b64840ba" | ||
| Combinatorics = "861a8166-3701-5b0c-9a16-15d98fcdc6aa" | ||
| DataGraphs = "b5a273c3-7e6c-41f6-98bd-8d7f1525a36a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dependency used? I removed anything requiring it a few version ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not used, I'll remove it.
Project.toml
Outdated
|
|
||
| [deps] | ||
| Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e" | ||
| CUDA = "052768ef-5323-5732-b1bb-66c8b64840ba" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a hard dependency (requires a GPU)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Project.toml
Outdated
| ITensors = "9136182c-28ba-11e9-034c-db9fb085ebd5" | ||
| KrylovKit = "0b1a1467-8014-51b9-945f-bf0ae24f4b77" | ||
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | ||
| MKL = "33e6dc65-8f57-5167-99aa-e5a354878fb2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| MKL = "33e6dc65-8f57-5167-99aa-e5a354878fb2" |
Project.toml
Outdated
| KrylovKit = "0b1a1467-8014-51b9-945f-bf0ae24f4b77" | ||
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | ||
| MKL = "33e6dc65-8f57-5167-99aa-e5a354878fb2" | ||
| NPZ = "15e1cf62-19b3-5cfa-8e77-841668bca605" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| NPZ = "15e1cf62-19b3-5cfa-8e77-841668bca605" |
Project.toml
Outdated
| NPZ = "15e1cf62-19b3-5cfa-8e77-841668bca605" | ||
| NamedGraphs = "678767b0-92e7-4007-89e4-4527a8725b19" | ||
| PauliPropagation = "293282d5-3c99-4fb6-92d0-fd3280a19750" | ||
| ProgressMeter = "92933f4c-e287-5a05-a399-4b506db050ca" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ProgressMeter = "92933f4c-e287-5a05-a399-4b506db050ca" |
| function update(alg::Algorithm"bp", bpc::AbstractBeliefPropagationCache) | ||
| compute_error = !isnothing(alg.kwargs.tolerance) | ||
| if !compute_error | ||
| println("ALERT, not computing error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This forces tolerance to always be defined. But it maybe be the case the user just wants to run BP for a fixed number of iterations and doesn't want to know convergence info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't force it to be defined, just prints a message. But sure I can remove it.
src/MessagePassing/loopcorrection.jl
Outdated
| end | ||
|
|
||
| #Get the all edges incident to the region specified by the vector of edges passed | ||
| # Optionally pass in a vector of vertices in the region, to handle the corner case where the region is just one vertex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you region is vertex based instead, this can be handled by boundary_edges(bpc::BeliefPropagationCache, verts::Vector) which should be an existing function (as there is a NamedGraphs version of it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, fixed it
| end | ||
| break | ||
| if compute_error | ||
| diffs[i] = diff.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diffs[i] is not being used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets printed out in line 222. In a previous version I was returning it along with bpc, but that seemed like too much of a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a print statement at the end of the function where the entire vector is printed. In an earlier version I also returned diffs, but that affects too many other functions so I opted not to.
| end | ||
|
|
||
| struct Loop | ||
| vertices::Vector{Int} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes the vertices are Int in the loop? But can't we label them arbitrarily like we do with a NamedGraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the cluster finding, we convert to a SimpleGraph first, which has integer vertices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I just changed to Vector anyway.
| is_proper_subset(a::RegionKey, b::RegionKey)::Bool | ||
| Return true if region `a` is a proper subset of `b`. | ||
| """ | ||
| function is_proper_subset(a::RegionKey, b::RegionKey)::Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the Julia function issubset not handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I just copied this from the code you shared with me before. Technically, issubset(a,b) also returns true if a==b, but I can just add that as a separate check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah could have
is_proper_subset(a::RegionKey, b::RegionKey) = issubset(a,b) && !issetequal(a,b)
|
Thanks Grace! How much work is the |
|
|
|
Yeah maybe it's best to drop the Hyperdualnumbers dependency then and go with the ratio approach? It's probably more efficient like you said and clearer from a code perspective (also removes a dependency) |
| This is overkill as it finds ALL subgraphs first, but my other implementation had bugs | ||
| """ | ||
| function enumerate_clusters(ng::NamedGraph, max_weight::Int; min_v::Int = 4, triangle_free::Bool = true, must_contain = [], min_deg::Int = 2, verbose::Bool = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this should definitely return the clusters with vertex labelling that ng has.
This is how I do it for edge_color (basically define the function on a SimpleGraph and then overload it on the NamedGraph by unwrapping the graph and re-wrapping the result:
function SimpleGraphAlgorithms.edge_color(g::AbstractNamedGraph, k::Int64)
pg, vs = position_graph(g), collect(vertices(g))
ec_dict = SimpleGraphAlgorithms.edge_color(UndirectedGraph(pg), k)
# returns k vectors of edges which each contain the colored/commuting edges
return [
[edgetype(g)(vs[first(first(e))], vs[last(first(e))]) for e in ec_dict if last(e) == i]
for i in 1:k
]
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I already handle the vertex labeling in the function generalized_loop_named? Is there an issue with the existing approach?
Added code for cluster expansion and cluster cumulant expansion. Calculation of one- and two-point functions. Example code in
examples/clustercorrections.jl.Note: generalized loop finding in cluster expansion relies on
nautywhich must be downloaded here.Other changes:
freenergytofree_energy.message_diffno longer returns negative numbers.Project.toml.