QMC code restructuring #433

Atomyka · 2022-10-01T01:52:26Z

Streamlining and restructuring QMC code.

removing extra file not needed.

codecov-commenter · 2022-10-01T01:56:36Z

Codecov Report

Merging #433 (d664212) into master (d281f22) will decrease coverage by 0.72%.
The diff coverage is 22.81%.

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
- Coverage   62.27%   61.55%   -0.73%     
==========================================
  Files          63       79      +16     
  Lines        3632     4789    +1157     
==========================================
+ Hits         2262     2948     +686     
- Misses       1370     1841     +471

Impacted Files	Coverage Δ
lib/BloqadeQMC/src/BloqadeQMC.jl	`0.00% <ø> (ø)`
lib/BloqadeQMC/src/datastructures/Rydberg.jl	`0.00% <0.00%> (ø)`
lib/BloqadeQMC/src/datastructures/alias.jl	`0.00% <0.00%> (ø)`
lib/BloqadeQMC/src/datastructures/hamiltonian.jl	`0.00% <0.00%> (ø)`
...oqadeQMC/src/datastructures/improved_op_sampler.jl	`0.00% <0.00%> (ø)`
lib/BloqadeQMC/src/datastructures/operators.jl	`0.00% <0.00%> (ø)`
...BloqadeQMC/src/datastructures/probabilityvector.jl	`0.00% <0.00%> (ø)`
lib/BloqadeQMC/src/datastructures/qmc_state.jl	`0.00% <0.00%> (ø)`
lib/BloqadeQMC/src/ising/TFIM.jl	`35.29% <ø> (+16.68%)`	⬆️
lib/BloqadeQMC/src/updates/multibranch.jl	`0.00% <0.00%> (ø)`
... and 32 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

weinbe58 · 2022-10-01T16:35:55Z

Looking good so far! BTW you have access to the QuEraComputing repo for Bloqade so you can push your own branches with name/branch_name

Roger-luo · 2022-10-01T23:07:55Z

lib/BloqadeQMC/src/updates/cluster.jl

+                First[st] = v + num_sites
+            end
+
+            @simd for i in 1:num_legs


Can we check if the simd is actually useful here? By measuring the time - simd optimization is not very reliable so it'd be great to check that while developing

Roger-luo · 2022-10-01T23:08:38Z

lib/BloqadeQMC/src/updates/cluster.jl

+        end
+    end
+
+    if qmc_state isa BinaryGroundState


I think this if else is an easy abstraction to separate out using multiple dispatch.

Roger-luo · 2022-10-01T23:09:37Z

lib/BloqadeQMC/src/updates/cluster.jl

+#############################################################################
+
+
+@inline function _map_back_operator_list!(ocount::Int, qmc_state::BinaryQMCState, H::AbstractIsing, d::Diagnostics)


I think we would just remove the beginning underscore from the name.

Roger-luo · 2022-10-01T23:12:09Z

lib/BloqadeQMC/src/updates/cluster.jl

+                    in_cluster[leg] = ccount  # add the new leg and flip it
+                    push!(current_cluster, leg)
+                    if qmc_state isa BinaryGroundState
+                        lnA += trialstate_weight_change(qmc_state, lsize, Ns, i)
+                    end
+                    a = Associates[leg]
+
+                    a == 0 && continue
+                    # from this point on, we know we're on a bond op
+                    op = operator_list[op_indices[leg]]
+                    w = getweightindex(H, op) - getoperatortype(H, op)
+                    preflip_bond_type, postflip_bond_type = update_kernel!(qmc_state, H, ccount, leg, a)
+                    lnA += (
+                        getlogweight(H.op_sampler, w + postflip_bond_type)
+                        - getlogweight(H.op_sampler, w + preflip_bond_type)
+                    )


I'd separate this to a function (at least to start with) it's always beneficial to wrap a loop kernel into a function

Roger-luo · 2022-10-01T23:14:30Z

lib/BloqadeQMC/src/updates/cluster.jl

+    ocount = _map_back_basis_states!(rng, lsize, qmc_state, H)
+    _map_back_operator_list!(ocount, qmc_state, H, d)
+
+    return lsize


It probably worth thinking if this return value can be a state object - it would be nice the MC iteration can roughly follow an iterator interface in Julia. But this is not a blocking comment.

Also it seems lsize is the same as input here? If I'm not missing something what's the point of returning the same value?

Roger-luo · 2022-10-01T23:17:54Z

lib/BloqadeQMC/src/updates/diagonal.jl

+
+    if !(runstats isa NoStats)
+        fit!(runstats, :diag_update_fails, failures/count)
+    end


Can we be explicit on what return value is of this function?

Roger-luo · 2022-10-01T23:18:13Z

lib/BloqadeQMC/src/updates/diagonal.jl

+    resize!(qmc_state.leg_sites, len)
+    resize!(qmc_state.op_indices, len)
+    resize!(qmc_state.in_cluster, len)
+end


The return value should be the state not the vector

Roger-luo · 2022-10-01T23:20:54Z

lib/BloqadeQMC/src/updates/cluster.jl

+    # Now, add the 2M operators to the linked list. Each has either 2 or 4 legs
+    @inbounds for (n, op) in enumerate(qmc_state.operator_list)
+        if issiteoperator(H, op)
+            site = getsite(H, op)


It would be nice if we can seperate out the code block for each different operator (site and bond)

Roger-luo · 2022-10-01T23:24:18Z

There are some general name conventions are not consistent with what we use in Bloqade. So it would be nice if you could just rename them when you spot them (so we will just check them in the end when the development is almost done). In general, we only use Camel case for types and all other things should be in snake case. And we don't usually use starting underscore to denote private function (unless you really having a hard time figuring out a proper name)

Atomyka · 2022-10-03T17:30:13Z

Haven't addressed comments above yet but the ltfim test is running now with restructured update rules :)

Atomyka · 2022-10-04T19:05:01Z

Suggestion for overall code structure:

datastructures/: the structs ProbabilityAlias and (Improved)OperatorSampler (and hence the hamiltonians) are all built on top of AbstractProbabilityVector. To make this clear, we could have them all in one file. For now, I also added qmc_state. Overall, this folder bundles the files where we need to think about integration with BloqadeExpr.
updates/: all QMC update rules
lattices/: to be removed since Phil & John have almost finished the necessary upgrades to BloqadeLattices
trialstate/: to be removed if we have no intention of combining the QMC ground state simulation with other variational algorithms
diagnostics/: we can keep it for now, mainly useful for us but definitely a blackbox for user (tracks cluster sizes, acceptance rates etc.)

The only top-level file left (apart from BloqadeQMC.jl) is still measurements.jl. Here, we need to discuss how many of the functions we really need for Bloqade. Will probably shrink significantly.

Atomyka · 2022-10-04T19:06:51Z

Also important: run a Rydberg QMC. Currently, the QMC test uses a LTFIM.

Code should be pretty much the same. And we can use the ED values from exact_diag.jl for comparison.

Atomyka · 2022-10-05T17:13:18Z

Note: Expansion order fluctuates in finite-T-case but stays fixed for zero-T.
In the current implementation, full_diagonal_update!() hence does not return anything, while full_diagonal_update_beta!() does return num_ops. This makes calling mc_step!() stlightly different from mc_step_beta!() --> Should we make them the same s.t. the user only has one interface (where she needs to specify beta = finite value or inf)

Atomyka · 2022-10-05T20:33:04Z

Also important: run a Rydberg QMC. Currently, the QMC test uses a LTFIM.

Code should be pretty much the same. And we can use the ED values from exact_diag.jl for comparison.

Got started on this in new PR

Atomyka · 2022-10-05T20:33:33Z

Note: Expansion order fluctuates in finite-T-case but stays fixed for zero-T. In the current implementation, full_diagonal_update!() hence does not return anything, while full_diagonal_update_beta!() does return num_ops. This makes calling mc_step!() stlightly different from mc_step_beta!() --> Should we make them the same s.t. the user only has one interface (where she needs to specify beta = finite value or inf)

Will leave this unchanged until we get properly started on user interface

weinbe58 · 2023-11-15T21:06:04Z

@Atomyka Can I close this PR?

Atomyka and others added 22 commits May 30, 2022 23:31

Test file

8d6a10c

Merge branch 'QuEraComputing:master' into master

dbf2fc2

Merge branch 'QuEraComputing:master' into master

90bc3d2

updated dependencies for testing jackknife function

b6deabf

added jackknife unit tests

95c4e9d

updated dependencies to include Jackknife

d096268

unit tests for jackknife resampling

e72c4f1

Delete test.jl

12261cb

removing extra file not needed.

Merge branch 'QuEraComputing:master' into master

f506721

updated comments defining the TFIM ops to match the code

034f8f4

more jackknife tests

7de44c7

removed Jackknife dependency

1828614

resolved error.jl merge conflict

14ff1d9

t-test with binning for single QMC run

7c8c851

Delete TFIM.jl

20a1447

Delete error.jl

4170937

fixed bug in energy binning

f376328

included ltfim.jl and error.jl in runtests.jl

0d9148d

readded error.jl

e9c73cc

readded TFIM.jl

f5a3aeb

modified N_eff calculation

226020b

restructured updated rules within new updates folder

dd67c24

Roger-luo reviewed Oct 1, 2022

View reviewed changes

Roger-luo requested changes Oct 1, 2022

View reviewed changes

Atomyka added 2 commits October 3, 2022 19:24

modified include structure to test new update files

82daf9d

copied over missing cluster_update!() function

6fb0f5f

Atomyka changed the title ~~QMC code restructuring~~ QMC updates restructuring Oct 4, 2022

Atomyka changed the title ~~QMC updates restructuring~~ QMC code restructuring Oct 4, 2022

file suggestions for grouping evth related to QMC datastructures

d664212

Merge branch 'QuEraComputing:master' into anna/code-restructuring

3e2d5f5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QMC code restructuring #433

QMC code restructuring #433

Atomyka commented Oct 1, 2022

codecov-commenter commented Oct 1, 2022 •

edited

Loading

weinbe58 commented Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo Oct 1, 2022

Roger-luo commented Oct 1, 2022

Atomyka commented Oct 3, 2022

Atomyka commented Oct 4, 2022

Atomyka commented Oct 4, 2022

Atomyka commented Oct 5, 2022

Atomyka commented Oct 5, 2022

Atomyka commented Oct 5, 2022

weinbe58 commented Nov 15, 2023

		#############################################################################


		@inline function _map_back_operator_list!(ocount::Int, qmc_state::BinaryQMCState, H::AbstractIsing, d::Diagnostics)

QMC code restructuring #433

Are you sure you want to change the base?

QMC code restructuring #433

Conversation

Atomyka commented Oct 1, 2022

codecov-commenter commented Oct 1, 2022 • edited Loading

Codecov Report

weinbe58 commented Oct 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roger-luo commented Oct 1, 2022

Atomyka commented Oct 3, 2022

Atomyka commented Oct 4, 2022

Atomyka commented Oct 4, 2022

Atomyka commented Oct 5, 2022

Atomyka commented Oct 5, 2022

Atomyka commented Oct 5, 2022

weinbe58 commented Nov 15, 2023

codecov-commenter commented Oct 1, 2022 •

edited

Loading