Skip to content

Add lattice-utils to dwave-experimental - #44

Open
SebastianGitt wants to merge 16 commits into
dwavesystems:mainfrom
SebastianGitt:add-lattice-utils
Open

Add lattice-utils to dwave-experimental#44
SebastianGitt wants to merge 16 commits into
dwavesystems:mainfrom
SebastianGitt:add-lattice-utils

Conversation

@SebastianGitt

Copy link
Copy Markdown
Contributor

Add a minimal version of LatQA to dwave-experimental as a submodule named lattice-utils. Currently supports examples of a 1D Ising Chain and a 2D dimerized triangular lattice. Tests still need to be added to the pull request.

-move initialization of num_spins to subclass

@thisac thisac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SebastianGitt! First pass of the lattice submodule.

  • I'd recommend running a formatter (black) since all of these files are new and there are a few places with incorrect spacing, etc.
  • I think the saving/loading of embeddings can be refined a bit, but I'll have a closer look at it later.

Comment thread dwave/experimental/lattice_utils/__init__.py Outdated
Comment thread dwave/experimental/lattice_utils/utils.py Outdated
Comment thread dwave/experimental/lattice_utils/utils.py Outdated
Comment thread dwave/experimental/lattice_utils/utils.py Outdated
Comment thread dwave/experimental/lattice_utils/utils.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/triangular.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/triangular.py
Comment thread dwave/experimental/lattice_utils/lattice/lattice.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/lattice.py
Comment thread dwave/experimental/lattice_utils/lattice/lattice.py
-added more comprehensive docstrings
-fixed formatting issues
-added more comprehensive docstrings
-fixed formatting issues
-removed spin reversal transform functionality
-uses automorphism module already in dwave-experimental
-uses data classes for experiment configs
-removed spin reversal transform functionality
-uses automorphism module already in dwave-experimental
-uses data classes for experiment configs
-formatted using black
@SebastianGitt
SebastianGitt marked this pull request as ready for review May 14, 2026 17:50

@andrew-d-king andrew-d-king left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good; I am not able to pore over it in detail but it all seems sensible.

Comment thread dwave/experimental/lattice_utils/experiment/experiment.py
Comment thread dwave/experimental/lattice_utils/experiment/experiment.py

return sampler_call

def _format_parameter_list(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not for this PR, but ultimately I think that a parameter class should be written, with a format parameter (basically sig figs, for float parameters). It's a bit ugly to do it ad hoc, as I've written it.

Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated

@thisac thisac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still have experiment module to look at. Re. the tests, it probably makes sense splitting them up into several files collected in a test_lattice_utils folder.

Comment thread dwave/experimental/lattice_utils/lattice/chain.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/lattice.py
Comment thread dwave/experimental/lattice_utils/lattice/lattice.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/lattice.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/embedded_lattice.py Outdated
Comment thread dwave/experimental/lattice_utils/observable/observable.py
Comment thread dwave/experimental/lattice_utils/observable/observable.py Outdated
Comment thread dwave/experimental/lattice_utils/observable/observable.py Outdated
Comment thread dwave/experimental/lattice_utils/observable/observable.py Outdated
Comment thread dwave/experimental/lattice_utils/observable/kinks.py Outdated
-added progress bars using TQDM
-added docstrings to the evaluation methods in observable.py
-misc cleanup
-added progress bars using TQDM
-added docstrings to the evaluation methods in observable.py
-misc cleanup

@jackraymond jackraymond left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, especially the integration of new automorphism functionality to make the shimming tutorial generalizable.

Some of this code is not lattice specific, I'd prefer it weren't bundled up in a lattice utility. Like embed_bqm (which mirrors a dwave.system) and unembed_bqm (which perhaps is, or should be dwave.system method), bootstrapping, bitstring compression. Ideally we could think how to extract it for more general use.

Examples don't run well, or at all for me. Adding an interface and some additional help messages on what is going on would be great. Perhaps cite papers etc where statistics are used.

There are a lot of magic numbers/methods floating around, if these could be promoted to function arguments and justified that would be great.

Some other naming conventions I found a bit confusing.

Comment thread dwave/experimental/lattice_utils/lattice/embedded_lattice.py
Comment thread dwave/experimental/lattice_utils/lattice/embedded_lattice.py Outdated
Comment thread dwave/experimental/lattice_utils/lattice/embedded_lattice.py
Comment thread dwave/experimental/lattice_utils/lattice/embedded_lattice.py
Comment thread dwave/experimental/lattice_utils/lattice/embedded_lattice.py
Comment thread dwave/experimental/lattice_utils/observable/observable.py
Comment thread dwave/experimental/lattice_utils/observable/observable.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_1D_Ising_chain.py Outdated
Comment thread examples/example_2D_geometric_frustration.py Outdated
-fix failing tests due to updated method signatures with explicit arguments
-rename energy_scale to signed_energy_scale
-remove existing examples and replaced with a new 1D chain shimming example
-restructure tests into separate files in a new folder
-update optimize function to accept custom dimod solvers
-expand docstring for ``embed_bqm`` method to explain difference from existing methods in dwave-system
-promote ``optimize`` and ``make_networkx_graph`` to public methods

@thisac thisac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far! A couple of comments and improvements before ready to merge.

One thing that we discussed already: tqdm should be made into an optional dependency. Currently it's required to install.


import dimod
import numpy as np
from tqdm.auto import tqdm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we say that we'd make tqdm optional?

Comment on lines +147 to +148
if not quiet:
print(f"Loaded {filename} at {datetime.now()}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now let's remove print statements from the package. We can think about either adding in logging messages if we really think it's needed, or warnings where reasonable.

Comment on lines +175 to +176
for param_name, param_val in param.items():
self.param[param_name] = param_val

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not doing

Suggested change
for param_name, param_val in param.items():
self.param[param_name] = param_val
self.param.update(param)

self.sampler = sampler
self.reference_energy_sampler = reference_energy_sampler
self.reference_energy_sampler_kwargs = reference_energy_sampler_kwargs
self.param = dict(vars(config))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vars returns a dict already

Suggested change
self.param = dict(vars(config))
self.param = vars(config)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done to return a copy so that future changes to self.param don't affect the config object

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use

Suggested change
self.param = dict(vars(config))
self.param = vars(config).copy()

instead? Both dict(vars(config)) and vars(config).copy() would just return shallow copies.

Comment on lines +151 to +152
if ignore_shim:
result_fields.remove("shimdata")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on docs it sounds like this should maybe be outside the if result_fields is None?

A numpy array containing the pairwise spin correlations for each coupler.
"""
sample_array = dimod.as_samples(sample_set)[0].astype(float)
if len(experiment.inst.edge_list) == 0:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should work and would in that case be recommended.

Suggested change
if len(experiment.inst.edge_list) == 0:
if not experiment.inst.edge_list:

A numpy array containing the mean coupler frustration for each edge.
"""
sample_array = dimod.as_samples(sample_set)[0].astype(float)
if len(experiment.inst.edge_list) == 0:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(experiment.inst.edge_list) == 0:
if not experiment.inst.edge_list:


return energy

def load(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few of the methods below are missing complete docstrings (load, save, and update).

Comment on lines +291 to +296
if new_energy < reference_energy:
if path is None:
path = get_reference_energy_path(bqm, experiment)
self.save(path, new_energy, sample, reference_method_string)
else:
raise ValueError("New energy is not better than reference energy, not updating.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but I'd turn this around

Suggested change
if new_energy < reference_energy:
if path is None:
path = get_reference_energy_path(bqm, experiment)
self.save(path, new_energy, sample, reference_method_string)
else:
raise ValueError("New energy is not better than reference energy, not updating.")
if new_energy >= reference_energy:
raise ValueError("New energy is not better than reference energy, not updating.")
if path is None:
path = get_reference_energy_path(bqm, experiment)
self.save(path, new_energy, sample, reference_method_string)

Comment on lines +2 to +8
features:
- |
Add ``dwave.experimental.lattice_utils`` submodule with utilities for
constructing lattice graphs (``Chain``, ``Triangular``,
``DimerizedTriangular``, ``EmbeddedLattice``), evaluating physics
observables on sample sets, and running shimmed Ising experiments on
QPU samplers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be split up into several features.

@thisac thisac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments...

if self.param.get("fast_anneal", False):
ret["fast_anneal"] = True

ret["annealing_time"] = self.param["anneal_time"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since elsewhere it seems like either "anneal_schedule" or "anneal_time" can be used, is this also the case here? If not, perhaps add a comment on why.

        if "anneal_schedule" in self.param:
            ...
        elif "anneal_time" in self.param:
            ret["annealing_time"] = self.param["anneal_time"]

for i in range(n - 1):
yield (i, i + 1)

if self.periodic[0] and n > 1:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be n > 2? As it currently stands, when self.dimensions[0] == 2 this would yield both (0, 1) and (1, 0).

Suggested change
if self.periodic[0] and n > 1:
if self.periodic[0] and n > 2:

Comment on lines +215 to +224
if qubit_orbits is not None and coupler_orbits is not None:
if len(qubit_orbits) != self.num_spins:
raise ValueError(
f"qubit_orbits must have length {self.num_spins}, got {len(qubit_orbits)}."
)
if len(coupler_orbits) != self.num_edges:
raise ValueError(
f"coupler_orbits must have length {self.num_edges}, "
f"got {len(coupler_orbits)}."
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If either qubit_orbits or coupler_orbits is None, then the other won't be checked (since this states and).

Should these checks be separated into:

if qubit_orbits and len(qubit_orbits) != self.num_spins:
    raise ValueError(
        f"qubit_orbits must have length {self.num_spins}, got {len(qubit_orbits)}."
    )

and

if coupler_orbits and len(coupler_orbits) != self.num_edges:
    raise ValueError(
        f"coupler_orbits must have length {self.num_edges}, "
        f"got {len(coupler_orbits)}."
    )

# See the License for the specific language governing permissions and
# limitations under the License.

"""Shimming example for 1D Ising chain."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also rename this file to just 1D_Ising_chain_shim.py or maybe prefix it with lattice_utils or even put it in a lattice_utils subfolder since there are quite a few more lattice-utils examples incoming, no?

Comment thread examples/example_1D_Ising_chain_shim.py Outdated

from dwave.system import DWaveSampler
import matplotlib.pyplot as plt
from matplotlib.colors import to_rgb

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from matplotlib.colors import to_rgb

Comment thread examples/example_1D_Ising_chain_shim.py Outdated
Comment on lines +25 to +26
from dwave.experimental.lattice_utils import lattice, experiment, observable
from dwave.experimental.lattice_utils.utils import bootstrap, confidence_interval

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used?

Suggested change
from dwave.experimental.lattice_utils import lattice, experiment, observable
from dwave.experimental.lattice_utils.utils import bootstrap, confidence_interval
from dwave.experimental.lattice_utils import lattice, experiment

Comment on lines +99 to +106
self.observables_to_collect = {
QubitMagnetization(),
CouplerCorrelation(),
CouplerFrustration(),
SampleEnergy(),
BitpackedSpins(),
ReferenceEnergy(),
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, does it even make sense having this be a set? Wouldn't a tuple be a more reasonable type? That nulls my other comment about not having to cast this to a set below (although I think not using sets at all here might make more sense).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it doesn't need to be a set, but for certain examples custom observables will be added to this, so it might be less clunky to use a list here instead of having to update the tuple with additional observables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably go for a list in that case.

-make TQDM an optional import
-order imports
-add some missing docstrings
-removed subscript unpacking for Python 3.10 compatibility
-updated licenses to 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.44944% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.74%. Comparing base (703b3cd) to head (2b02cee).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...xperimental/lattice_utils/experiment/experiment.py 83.67% 64 Missing ⚠️
...wave/experimental/lattice_utils/lattice/lattice.py 89.55% 14 Missing ⚠️
...e/experimental/lattice_utils/lattice/triangular.py 91.95% 7 Missing ⚠️
...xperimental/lattice_utils/observable/observable.py 92.70% 7 Missing ⚠️
...rimental/lattice_utils/lattice/embedded_lattice.py 95.65% 4 Missing ⚠️
...xperimental/lattice_utils/observable/triangular.py 84.00% 4 Missing ⚠️
dwave/experimental/lattice_utils/lattice/chain.py 95.00% 1 Missing ⚠️
dwave/experimental/lattice_utils/utils.py 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
- Coverage   95.38%   92.74%   -2.64%     
==========================================
  Files          13       30      +17     
  Lines         931     1999    +1068     
==========================================
+ Hits          888     1854     +966     
- Misses         43      145     +102     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Successfully merging this pull request may close these issues.

6 participants